feat(core): Deprecate span tags, data, context & setters
#10053
Conversation
| @@ -159,6 +159,8 @@ export function startProfileForTransaction(transaction: Transaction): Transactio | |||
| // Always call onProfileHandler to ensure stopProfiling is called and the timeout is cleared. | |||
| void onProfileHandler().then( | |||
| () => { | |||
| // TODO: Can we rewrite this to use attributes? | |||
| // eslint-disable-next-line deprecation/deprecation | |||
| transaction.setContext('profile', { profile_id: profileId, start_timestamp: startTimestamp }); | |||
There was a problem hiding this comment.
cc @JonasBa we either need to move this to be an attribute (~~data) on the transaction, or find a way to set these on the surrounding scope of the transaction instead 🤔 nothing to do in this PR but when we actually remove this in v8 we need to handle this somehow.
There was a problem hiding this comment.
Ok sounds good. Thanks for the ping!
| @@ -145,6 +145,7 @@ export class IdleTransaction extends Transaction { | |||
| this.activities = {}; | |||
|
|
|||
| if (this.op === 'ui.action.click') { | |||
| // eslint-disable-next-line deprecation/deprecation | |||
| this.setTag(FINISH_REASON_TAG, this._finishReason); | |||
There was a problem hiding this comment.
Do we need this tag on the idle transaction in v8? @AbhiPrasad
There was a problem hiding this comment.
OK, I'll just change it to an attribute then!
packages/core/src/utils/spanUtils.ts
Outdated
| import { dropUndefinedKeys, generateSentryTraceHeader } from '@sentry/utils'; | ||
|
|
||
| /** | ||
| * Convert a span to a trace context, which can be sent as the `trace` context in an event. | ||
| */ | ||
| export function spanToTraceContext(span: Span): TraceContext { | ||
| const { data, description, op, parent_span_id, span_id, status, tags, trace_id, origin } = span.toJSON(); | ||
| const { |
There was a problem hiding this comment.
This conflicted with some tests, but I figured it is overall safer as we assumed that this always is a Span class instance with toJSON() which may be a bit unsafe. Now we just treat this as a POJO which should be safer (and .toJSON() will actually go away for v8 as well, so we may as well already do that now)
| // Since formDataObject is not a primitive, we cannot store it on the span as attributes | ||
| // so instead we put it as extra on the scope | ||
| const scope = getCurrentScope(); | ||
| scope.setExtra('server_action_form_data', formDataObject); |
There was a problem hiding this comment.
@lforst do we need this as a nested object here? attributes cannot be nested, so going forward we need to either flatten this, if the shape is known (which I guess it is not?), or store it as context on the scope (?), or as JSON string...
| @@ -341,6 +341,8 @@ export function trpcMiddleware(options: SentryTrpcMiddlewareOptions = {}) { | |||
| trpcContext.input = normalize(rawInput); | |||
| } | |||
|
|
|||
| // TODO: Can we rewrite this to an attribute? Or set this on the scope? | |||
| // eslint-disable-next-line deprecation/deprecation | |||
| sentryTransaction.setContext('trpc', trpcContext); | |||
There was a problem hiding this comment.
Any thoughts on this? Not sure how important that is, but I guess we need some way to access the scope of the sentryTransaction then we could just put it on there...?
| @@ -201,17 +200,14 @@ function updateSpanWithOtelData(sentrySpan: SentrySpan, otelSpan: OtelSpan): voi | |||
| function updateTransactionWithOtelData(transaction: Transaction, otelSpan: OtelSpan): void { | |||
| const { op, description, source, data } = parseOtelSpanDescription(otelSpan); | |||
|
|
|||
| // eslint-disable-next-line deprecation/deprecation | |||
| transaction.setContext('otel', { | |||
There was a problem hiding this comment.
I guess this will generally go away in v8, so no need to worry about this.
Which reminds me, we should also deprecate these I guess...
size-limit report 📦
|
mydea
force-pushed
the
fn/spanData
branch
2 times, most recently
from
5ebab8f
to
6cfe78b
Compare
| } else { | ||
| formDataObject[key] = '[non-string value]'; | ||
| } | ||
| span?.setAttribute( |
There was a problem hiding this comment.
After talking with @lforst, we decided to flatten this here, as the exact shape of this is not important, and thus we can more easily rewrite it to attributes!
mydea
force-pushed
the
fn/spanData
branch
2 times, most recently
from
30dc0ac
to
4cb5aaa
Compare
|
I updated this to use |
Also deprecate direct access to `span.attributes` (instead use `spanGetAttributes(span)`). There are a few usages that we still need to figure out how we will replace them.
Also deprecate direct access to
span.attributes(instead usespanToJSON(span)).There are a few usages that we still need to figure out how we will replace them...!