Uh oh!
There was an error while loading. Please reload this page.
feat(node): Add context info for missing instrumentation - #12639
Conversation
andreiborza
commented
Jun 25, 2024
Could we add a test for this? |
| } | ||
| }); | ||
| getCurrentScope().setContext('Instrumentation', { |
There was a problem hiding this comment.
m: This should be getGlobalScope(), I'd say - the current scope is very "volatile", putting this on the global scope ensures it will always be there!
There was a problem hiding this comment.
Was not sure about the scope, for the case if we have multiple frameworks in a project, would they overwrite each other?
There was a problem hiding this comment.
yes, the last would win! IMHO that's probably OK, but maybe you can also check with other folks on the team what they think about this :)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } | ||
| }); | ||
| getGlobalScope().setContext('Instrumentation', { |
There was a problem hiding this comment.
| getGlobalScope().setContext('Instrumentation',{ | |
| getGlobalScope().setContext('instrumentation',{ |
If we are adding new context, let's please make sure this is documented in https://develop.sentry.dev/sdk/event-payloads/contexts/
I also fear that instrumentation is a pretty big namespace to take, and could collide with custom contexts relatively easily. Could we think about a name that potentially has less collisions?
We can discuss naming, and evaluate what exact fields should be here in the PR to develop https://develop.sentry.dev/sdk/event-payloads/contexts/. We might want to re-evaluate isCjs for example because it is JS specific, and contexts should be agnostic to language as much as possible.
There was a problem hiding this comment.
alternate is to namespace the context javascript_instrumentation - but we should still make sure it shows up in develop.
There was a problem hiding this comment.
What about missing_instrumentation?
If we exclude the isCjs, it might make sense to replace the new context altogether with just a tag?
There was a problem hiding this comment.
Or have a generic extra object? Not sure if that's compatible tho, just thinking out loud.
There was a problem hiding this comment.
I like missing_instrumentation. isCjs does seems important, but we can get around this by namespacing the context keys with javascript.X to indicate language specific instrumentation errors.
contexts also tend to follow snake case as they are formatted for the backend.
interfaceMissingInstrumentationContext{// Note: This seems redundant because the existence of `missing_instrumentation` context should// indicate `is_missing: true`is_missing: boolean;// package name// this should probably be requiredpackage: string;['javascript.is_cjs']?: boolean;}The SDK should avoid setting tags as much as possible because tags are meant for user-set values. In fact tags we "automatically" add like browser and os are actually converted from contexts to tags by the sentry backend during event ingestion. Tags are indexed and searchable in our backend, so setting new one's automatically puts a burden on the infrastructure that we'd much rather be able to control dynamically (hence the mechanism to "promote" certain contexts to tags like we do for browser).
extra also makes sense, but it is soft-deprecated, and the product/relay does not have code (like it does for contexts) to handle it.
Hence I think picking contexts is the right vehicle for this because we can easily adapt relay/frontend/backend in the product-side to read contexts and use them, and it makes sense for the product to use these values for better onboarding/troubleshooting.
There was a problem hiding this comment.
Got it, thanks for the clarification on tags!
c06cca0 to
eae9855CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Adds context to the scope for missing instrumentation of node frameworks.
Fixes#12346