refactor: consolidate platform-specific code#6208
refactor: consolidate platform-specific code#6208overbalance merged 8 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6208 +/- ##
==========================================
- Coverage 95.40% 95.39% -0.02%
==========================================
Files 317 314 -3
Lines 9514 9513 -1
Branches 2192 2194 +2
==========================================
- Hits 9077 9075 -2
- Misses 437 438 +1
🚀 New features to boost your workflow:
|
Even though it's nice to have shared code, I'm not sure we should make this method slower. I (and others) am already seeing drastic slowdowns in HTTP servers when OTel is enabled, and |
3e120ec to
16a4d1a
Compare
|
@cjihrig Cool. Reverted. |
16a4d1a to
b4bebe0
Compare
b4bebe0 to
e72efa4
Compare
david-luna
left a comment
There was a problem hiding this comment.
I like the idea of consolidating code whenever possible without affecting perf. In fact I think sdk-info.ts from core package could be consolidated as well. LGTM but let's wait for other reviews :)
pichlermarc
left a comment
There was a problem hiding this comment.
opentelemetry-core
- globalThis: Moved to
common- browser's fallback chain (globalThis → self → window → global → {}) works in all environments
We should be able to deprecate this and subsequently remove it in @opentelmetry/core@3.0.0, actually. It's only used in @opentelemetry/instrumentation-fetch
suggestion: let's copy the browser's fallback chain to where it's used in @opentelemetry/instrumentation-fetch and deprecate the one in @opentelemetry/core.
|
@pichlermarc I updated api and api-logs as well since they had the same pattern and the docs in core said to keep api in sync. |
| typeof process.argv0 === 'string' && | ||
| process.argv0.length > 0 | ||
| ? `unknown_service:${process.argv0}` | ||
| : 'unknown_service'; |
There was a problem hiding this comment.
@overbalance
This breaks on vercel edge environments
There was a problem hiding this comment.
Hi @overbalance, as @chargome mentioned, this unfortunately breaks our vercel-edge environment due to process.argv0 being a node api.
We ended up having to bundle in @opentelemetry/resources and any code that uses it. It's not ideal.
Any way we can figure out a workaround here?
There was a problem hiding this comment.
@chargome @andreiborza check out this other PR I've had out for review. Do you think it would solve your issue: #6257
There was a problem hiding this comment.
hey, that might work! Thanks for tackling this 🙏
There was a problem hiding this comment.
@andreiborza You said this "breaks"? What is the break? Inferring from @overbalance's PR there is warning from vercel-edge static analysis of some sort? Can you show an example?
For example, I don't recall here about warnings/breakages from this old change that was feature-testing for Node.js' process object: https://github.com/open-telemetry/opentelemetry-js/pull/4604/files#diff-7775cbc8f9eed30ff77488613bcbabec57446d86f655166fbdb5f7db4579a0b6R29
In that case it was checking global.process?.<something>.
If the code here in this PR used global.process instead of process, would that make the static analysis warnings go away?
There was a problem hiding this comment.
Here's an example run of a nextjs app using our vercel-edge SDK: https://github.com/getsentry/sentry-javascript/actions/runs/20844816035/job/59886209607?pr=18734#step:16:157
We end up getting
'A Node.js API is used (process.argv0 at line: 19) which is not supported in the Edge Runtime.\n' +
issues when building the app.
There was a problem hiding this comment.
Thanks @andreiborza I was able to replicate and solve the issue: #6257
Which problem is this PR solving?
Platform directories contain duplicate code with fallback chains for legacy environments that are no longer needed. Since the minimum supported versions are Node 18+ and modern browsers,
globalThisandperformanceare universally available.Short description of the changes
@opentelemetry/api
src/platform/directoryglobalThisdirectly instead of_globalThis@opentelemetry/api-logs
src/platform/directoryglobalThisdirectly instead of_globalThis@opentelemetry/core
common/globalThis.tsas deprecated re-export ofglobalThisotperformanceis now a deprecated alias forperformanceglobalThis.tsandperformance.tsfiles@opentelemetry/resources
defaultServiceNametosrc/with runtime check (process.argv0in Node, fallback tounknown_service)src/platform/directorydefaultServiceName@opentelemetry/instrumentation-fetch
globalThisdirectly instead of importing_globalThisfrom coreDocumentation
Type of change
How Has This Been Tested?
npm testpasses for all affected packagesdefaultServiceName(Node + browser environments)Checklist: