feat(node): Add useOperationNameForRootSpan tographqlIntegration
#13248
Conversation
| @@ -22,7 +22,7 @@ export function spanHasAttributes<SpanType extends AbstractSpan>( | |||
| */ | |||
| export function spanHasKind<SpanType extends AbstractSpan>(span: SpanType): span is SpanType & { kind: SpanKind } { | |||
| const castSpan = span as ReadableSpan; | |||
| return !!castSpan.kind; | |||
| return 'kind' in castSpan; | |||
There was a problem hiding this comment.
Noticed this was actually incorrect because this may be 0!
size-limit report 📦
|
|
Should we be concerned about transaction name cardinality here? This is a client-controlled field supporting arbitrary values. I'm not sure |
|
@mjq I would consider gql queries like these to be low cardinality. Why would you say this is hight cardinality? I think this is comparable to normal api route paths (?). |
I agree - remember this is only using the operation name, not the query. So e.g. if your query is this: query MyQuery(id) {
user(id): User
}it would only capture |
|
@lforst @mydea If your only client is internal, it ought to be low cardinality and work well as a grouping mechanism. If it's a public API, every caller will use their own query names, meaning cardinality is effectively unbounded (and it seems less useful as a grouping mechanism since you don't control the grouping yourself). And every API is de facto a public API without further defence mechanisms 😅 The main differences from e.g. rest API URLs is that a well-configured SDK is immune to high cardinality no matter what a client sends, and a poorly-configured one can still have its cardinality reduced by our URL normalizer. It's fine if the answer is "no we shouldn't be concerned", and treat this as an unlikely hypothetical. We aren't forwarding the cardinality cost to users so e.g. malicious use only affects Sentry, not our users' wallets, and we're giving anyone with concerns an off switch. |
|
Ah, I see what you mean. |
| @@ -22,7 +22,7 @@ export function spanHasAttributes<SpanType extends AbstractSpan>( | |||
| */ | |||
| export function spanHasKind<SpanType extends AbstractSpan>(span: SpanType): span is SpanType & { kind: SpanKind } { | |||
| const castSpan = span as ReadableSpan; | |||
| return !!castSpan.kind; | |||
| return typeof castSpan.kind === 'number'; | |||
There was a problem hiding this comment.
sorry, had a comment here earlier but changed it again - this check was actually wrong, because this can be 0 which is valid, but we interpreted this as no kind before.
mydea
force-pushed
the
fn/graphql-root-spans
branch
from
707babf to
f71275c
Compare
|
I added a docs issue for this as this is not documented yet: getsentry/sentry-docs#12080 |
This introduces a new option for the
graphqlIntegration,useOperationNameForRootSpan, which is by defaulttruebut can be disabled in integration settings like this:Sentry.graphqlIntegration({ useOperationNameForRootSpan: true }).With this setting enabled, the graphql instrumentation will update the
http.serverroot span it is in (if there is one) will be appended to the span name. So instead of having all root spans bePOST /graphql, the names will now be e.g.POST /graphql (query MyQuery).If there are multiple operations in a single http request, they will be appended like
POST /graphql (query Query1, query Query2), up to a limit of 5, at which point they will be appended as+2or similar.Closes #13238