-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core): Deprecate transaction metadata in favor of attributes #10097
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was also not 100% sure. I modeled this after OTEL, which does it the same way. 🤔
We could do:
export const SENTRY_ATTR_SOURCE = 'sentry.source';
export const SENTRY_ATTR_SAMPLE_RATE = 'sentry.sample_rate';
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably just do SEMANTIC_ATTRIBUTE_SENTRY_SOURCE
. Abbreviations feel weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otel probably didn't consider this because they don't think too much about browser land I imagine. We will have to consider this.
packages/core/src/tracing/span.ts
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good!
packages/core/src/tracing/trace.ts
Outdated
/** | ||
* 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn that's crazy/scary. It's fine to keep it then!
32a5352
to
f49dfc2
Compare
I updated this with new semantic attribute constants! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for integrating my suggestions!
15fbba1
to
8257765
Compare
@@ -17,12 +17,12 @@ sentryTest('should be able to handle circular data', async ({ getLocalTestPath, | |||
|
|||
expect(eventData.contexts).toMatchObject({ | |||
trace: { | |||
data: { lays: { contains: '[Circular ~]' } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be a good thing to put under a subpath export @sentry/core/attributes
, let's give it a try in v8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I'll wait with #10094 until this is merged to update reading the sample rate
better semantic attributes & fix tests
8257765
to
4d9763a
Compare
So we do not mutate this if we update data there. Noticed this here: #10097
This deprecates any usage of
metadata
on transactions.The main usages we have are to set
sampleRate
andsource
in 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