Uh oh!
There was an error while loading. Please reload this page.
feat(core): Deprecate transaction metadata in favor of attributes - #10097
Conversation
size-limit report 📦
|
lforst
left a comment
There was a problem hiding this comment.
generally lgtm - only minor stuff but the one with the dict may have long term impact so we should resolve it.
| export const SentrySemanticAttributes = { | ||
| Source: 'sentry.source', | ||
| SampleRate: 'sentry.sample_rate', | ||
| } as const; |
There was a problem hiding this comment.
m: I have a slight concern here. This object is not tree shakable, in the sense that as soon as it grows, whenever you use one value, the entire object is pulled into the bundle.
I would suggest we either a) expose the attribute consts as individual variables, b) create individual variables and put it under some name space that is still tree-shakable.
There was a problem hiding this comment.
Yeah, I was also not 100% sure. I modeled this after OTEL, which does it the same way. 🤔
We could do:
exportconstSENTRY_ATTR_SOURCE='sentry.source';exportconstSENTRY_ATTR_SAMPLE_RATE='sentry.sample_rate';?
There was a problem hiding this comment.
I would probably just do SEMANTIC_ATTRIBUTE_SENTRY_SOURCE. Abbreviations feel weird.
There was a problem hiding this comment.
Otel probably didn't consider this because they don't think too much about browser land I imagine. We will have to consider this.
| const idStr = childSpan.transaction.spanId; | ||
| const logMessage = `[Tracing] Starting '${opStr}' span on transaction '${nameStr}' (${idStr}).`; | ||
| childSpan.transaction.metadata.spanMetadata[childSpan.spanId] = { logMessage }; | ||
| logger.log(logMessage); | ||
| this._logMessage = logMessage; | ||
| } |
There was a problem hiding this comment.
I vote we kill this entire log-message-on-span thing. I am not sure it is worth the complexity. The actual logger log message should stay though probably.
There was a problem hiding this comment.
yeah, I generally agree, problem is we need the transaction name when finishing, where we don't really have access to this. IMHO this is OK for now and we can look to further simplify this when we do more work on spans (I don't want to spend too much time on this right now because... there are a million other things to do as well xD)
There was a problem hiding this comment.
Note that now the span simply keeps it's own log message so we can re-use it when it is ended, so it should already be much simpler than it was before 😅
| /** | ||
| * Metadata associated with the transaction, for internal SDK use. | ||
| * @deprecated Use attributes or store data on the scope instead. | ||
| */ | ||
| metadata?: Partial<TransactionMetadata>; |
There was a problem hiding this comment.
Can you explain the thought process behind adding a deprecated field? Also, it looks like TransactionContext already has this field. If it's just for a slightly different JSDoc, we should say "with the span" here right?
There was a problem hiding this comment.
this is already added, but the eslint deprecation rule does not seem to pick up inherited deprecated fields 😬 so without this, it does not show up as deprecated. This is generally very sad...
There was a problem hiding this comment.
Damn that's crazy/scary. It's fine to keep it then!
32a5352 to
f49dfc2Comparemydea
commented
Jan 8, 2024
I updated this with new semantic attribute constants! |
lforst
left a comment
There was a problem hiding this comment.
Thanks for integrating my suggestions!
15fbba1 to
8257765Compare| expect(eventData.contexts).toMatchObject({ | ||
| trace: { | ||
| data: { lays: { contains: '[Circular ~]' } }, |
There was a problem hiding this comment.
Hah, this started failing because it depended on no other data being added to the transaction - but now under the hood it adds sample rate attribute, which lead to this not being the same at the root and one level deeper nesting. Making this an explicit data entry makes this test clearer IMHO.
| * Should be one of: custom, url, route, view, component, task, unknown | ||
| * | ||
| */ | ||
| export const SEMANTIC_ATTRIBUTE_SENTRY_SOURCE = 'sentry.source'; |
There was a problem hiding this comment.
might be a good thing to put under a subpath export @sentry/core/attributes, let's give it a try in v8.
better semantic attributes & fix tests
8257765 to
4d9763aCompareSo we do not mutate this if we update data there. Noticed this here: #10097
This deprecates any usage of
metadataon transactions.The main usages we have are to set
sampleRateandsourcein there. These I replaced with semantic attributes. For backwards compatibility, when creating the transaction event we still check the metadata there as well.Other usage of metadata (mostly around
request) remains intact for now, we need to replace this in v8 - e.g. put this on the isolation scope, probably.This is the first usage of Semantic Attributes in the SDK!
This replaces #10041