feat(node): Add context info for missing instrumentation #12639
Conversation
|
Could we add a test for this? |
| @@ -24,5 +24,10 @@ export function ensureIsWrapped( | |||
| ); | |||
| } | |||
| }); | |||
|
|
|||
| 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 :)
dev-packages/e2e-tests/test-applications/node-express-incorrect-instrumentation/src/app.ts
Show resolved
Hide resolved
| @@ -24,5 +24,11 @@ export function ensureIsWrapped( | |||
| ); | |||
| } | |||
| }); | |||
|
|
|||
| 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.
interface MissingInstrumentationContext {
// 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 required
package: 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!
chargome
force-pushed
the
cg/add-instrumentation-info-to-ctx
branch
from
c06cca0 to
eae9855
Compare
Adds context to the scope for missing instrumentation of node frameworks.
Fixes #12346