refactor(resources): use runtime check for default service name#6257
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6257 +/- ##
==========================================
+ Coverage 95.41% 95.42% +0.01%
==========================================
Files 317 317
Lines 9597 9599 +2
Branches 2220 2217 -3
==========================================
+ Hits 9157 9160 +3
+ Misses 440 439 -1
🚀 New features to boost your workflow:
|
ec46c10 to
9a1d2ff
Compare
|
Unofficial LGTM, thanks! |
|
@cjihrig I unofficially accept 😅 |
| typeof process.argv0 === 'string' && | ||
| process.argv0.length > 0 | ||
| ? `unknown_service:${process.argv0}` | ||
| : 'unknown_service'; |
There was a problem hiding this comment.
I asked at #6208 (comment) whether potentially using global.process?.argv0 would suffice to avoid whatever static analysis warnings.
It would also be nice to have specific examples of these static analysis warnings to help for motivation in future maintenance.
|
Does the added vercel bundler test actually test a failure here? I reverted the implementation: diff --git a/packages/opentelemetry-resources/src/default-service-name.ts b/packages/opentelemetry-resources/src/default-service-name.ts
index 88b236379..083e7ddb5 100644
--- a/packages/opentelemetry-resources/src/default-service-name.ts
+++ b/packages/opentelemetry-resources/src/default-service-name.ts
@@ -14,26 +14,18 @@
* limitations under the License.
*/
-let serviceName: string | undefined;
+const DEFAULT_SERVICE_NAME =
+ typeof process === 'object' &&
+ typeof process.argv0 === 'string' &&
+ process.argv0.length > 0
+ ? `unknown_service:${process.argv0}`
+ : 'unknown_service';
-/**
- * Returns the default service name for OpenTelemetry resources.
- * In Node.js environments, returns "unknown_service:<process.argv0>".
- * In browser/edge environments, returns "unknown_service".
- */
export function defaultServiceName(): string {
- if (serviceName === undefined) {
- try {
- const argv0 = globalThis.process.argv0;
- serviceName = argv0 ? `unknown_service:${argv0}` : 'unknown_service';
- } catch {
- serviceName = 'unknown_service';
- }
- }
- return serviceName;
+ return DEFAULT_SERVICE_NAME;
}
/** @internal For testing purposes only */
export function _clearDefaultServiceNameCache(): void {
- serviceName = undefined;
+ // serviceName = undefined;
}re-ran |
255c905 to
6b84134
Compare
|
@trentm I added 2 edge runtime integration tests that fail on build warnings. I tested them by replacing
|
506840f to
722af49
Compare
|
Here's a failing example run: https://github.com/open-telemetry/opentelemetry-js/actions/runs/20980919971/job/60305548666 |
494c163 to
4acde2a
Compare
|
The functional change looks good. IIUC, this is blocking some folks working on browsers, so we should get this in. next-16 bundler test@overbalance Do I have it right that the next-16 bundler test is effectively a test of using Turbopack? ... I guess is about whatever the default config of Turbopack is in next-16. For all I know turbopack only has a life together with next@16. If so, this bundler test sounds good to me. Output from the linked example test failure: next-15 bundler testSo this is effectively a test of webpack. Do you think this could be done by tweaking the existing webpack-5 bundler test? So, I've no idea how (Per my message above about getting this in, I'd be happy to get this in and discuss the next-15 bundler test in a follow-up PR.) |
trentm
left a comment
There was a problem hiding this comment.
LGTM.
Happy to get this in, though I do have a Q about the bundler tests.
Let me know if we should just get this in right away at get a release out. I'm a little unsure of the urgency.
@trentm Next is doing a little bit of magic here beyond standard webpack options, like fully rejecting node api access. There may be a way to have smaller tests that leverage https://edge-runtime.vercel.app/packages/jest-environment but I haven't explored it yet. |
|
Oh, and regarding the urgency I would ask @chargome and @andreiborza for input. It looks like Sentry had to prebundle to keep edge runtime support. |
|
@overbalance correct, we ended up bundling the package in and duplicating apis from other SDKs. We are not blocked by this since we have this work-around but it's rather brittle and we'd love to remove the duplication and bundling again. Thanks again for taking care of this 🙏 |
| return DEFAULT_SERVICE_NAME; | ||
| if (serviceName === undefined) { | ||
| try { | ||
| const argv0 = globalThis.process.argv0; |
There was a problem hiding this comment.
@overbalance this is actually still preventing our builds to go through 😓
we still get the same warnings as before:
./node_modules/.pnpm/@opentelemetry+resources@2.4.0_@opentelemetry+api@1.9.0/node_modules/@opentelemetry/resources/build/esm/default-service-name.js A Node.js API is used (process.argv0 at line: 19) which is not supported in the Edge Runtime.
Which I guess makes sense given this is statically analysed.
Oops, hasn't been released yet. My bad 😅.
There was a problem hiding this comment.
No worries. Give it a shot now @andreiborza
There was a problem hiding this comment.
It worked: getsentry/sentry-javascript#18934
Thanks for taking care of this!
There was a problem hiding this comment.
I saw your comment on another PR about unclear Edge Runtime support. The Browser SIG has not discussed it but I'll bring it up on Slack to see if the JS SIG has. Feel free to join us there.



Which problem is this PR solving?
Static analysis tools in browser/Edge Runtime environments warn about accessing
process.argv0directly. Edge runtimes may also define aprocessobject that is empty or incomplete.Short description of the changes
globalThis.process.argv0with try/catch to handle environments whereprocessmay not exist or may be incomplete_clearDefaultServiceNameCache()helper for testingType of change
How Has This Been Tested?
processis undefined, empty, or fully populatedChecklist: