feat(core): Deprecate transaction metadata in favor of attributes by mydea · Pull Request #10097 · getsentry/sentry-javascript

mydea · 2024-01-08T12:54:45Z

This deprecates any usage of metadata on transactions.

The main usages we have are to set sampleRate and source 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

github-actions · 2024-01-08T13:04:41Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped)	76.57 KB (+0.12% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	67.94 KB (+0.08% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped)	61.57 KB (+0.11% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	32.18 KB (+0.18% 🔺)
@sentry/browser (incl. Feedback) - Webpack (gzipped)	30.7 KB (0%)
@sentry/browser - Webpack (gzipped)	22.07 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped)	74.21 KB (+0.16% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	65.84 KB (+0.14% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	32 KB (+0.21% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped)	23.76 KB (0%)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	206.8 KB (+0.14% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	96.66 KB (+0.27% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	70.93 KB (0%)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	34.97 KB (+0.24% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	68.31 KB (+0.09% 🔺)
@sentry/react - Webpack (gzipped)	22.09 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	85.03 KB (+0.08% 🔺)
@sentry/nextjs Client - Webpack (gzipped)	49.39 KB (+0.13% 🔺)
@sentry-internal/feedback - Webpack (gzipped)	16.74 KB (0%)

lforst

generally lgtm - only minor stuff but the one with the dict may have long term impact so we should resolve it.

lforst · 2024-01-08T13:10:29Z

packages/core/src/tracing/spanAttributes.ts

+export const SentrySemanticAttributes = {
+  Source: 'sentry.source',
+  SampleRate: 'sentry.sample_rate',
+} as const;


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.

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';

?

I would probably just do SEMANTIC_ATTRIBUTE_SENTRY_SOURCE. Abbreviations feel weird.

Otel probably didn't consider this because they don't think too much about browser land I imagine. We will have to consider this.

lforst · 2024-01-08T13:13:16Z

packages/core/src/tracing/span.ts

      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;
    }


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.

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)

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 😅

sounds good!

lforst · 2024-01-08T13:13:52Z

packages/core/src/tracing/trace.ts

+  /**
+   * Metadata associated with the transaction, for internal SDK use.
+   * @deprecated Use attributes or store data on the scope instead.
+   */
+  metadata?: Partial<TransactionMetadata>;


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?

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...

Damn that's crazy/scary. It's fine to keep it then!

mydea · 2024-01-08T15:27:39Z

I updated this with new semantic attribute constants!

lforst

Thanks for integrating my suggestions!

mydea · 2024-01-08T16:15:51Z

dev-packages/browser-integration-tests/suites/public-api/startTransaction/circular_data/test.ts


  expect(eventData.contexts).toMatchObject({
    trace: {
-      data: { lays: { contains: '[Circular ~]' } },


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.

AbhiPrasad · 2024-01-08T16:41:57Z

packages/core/src/semanticAttributes.ts

+ * Should be one of: custom, url, route, view, component, task, unknown
+ *
+ */
+export const SEMANTIC_ATTRIBUTE_SENTRY_SOURCE = 'sentry.source';


might be a good thing to put under a subpath export @sentry/core/attributes, let's give it a try in v8.

Lms24

Nice! I'll wait with #10094 until this is merged to update reading the sample rate

better semantic attributes & fix tests

So we do not mutate this if we update data there. Noticed this here: #10097

mydea requested review from AbhiPrasad, Lms24 and lforst January 8, 2024 12:54

mydea self-assigned this Jan 8, 2024

lforst reviewed Jan 8, 2024

View reviewed changes

mydea force-pushed the fn/deprecate-spanMetadata branch from 32a5352 to f49dfc2 Compare January 8, 2024 15:27

lforst approved these changes Jan 8, 2024

View reviewed changes

mydea force-pushed the fn/deprecate-spanMetadata branch 2 times, most recently from 15fbba1 to 8257765 Compare January 8, 2024 16:14

mydea commented Jan 8, 2024

View reviewed changes

mydea mentioned this pull request Jan 8, 2024

fix(core): Ensure we copy passed in span data/tags/attributes #10105

Merged

AbhiPrasad approved these changes Jan 8, 2024

View reviewed changes

Lms24 approved these changes Jan 8, 2024

View reviewed changes

mydea added 2 commits January 9, 2024 08:54

feat(core): Deprecate transaction metadata in favor of attributes

84c7991

better semantic attributes & fix tests

fix test

4d9763a

mydea force-pushed the fn/deprecate-spanMetadata branch from 8257765 to 4d9763a Compare January 9, 2024 08:34

mydea merged commit 0276c03 into develop Jan 9, 2024

mydea deleted the fn/deprecate-spanMetadata branch January 9, 2024 08:58

mydea added a commit that referenced this pull request Jan 9, 2024

fix(core): Ensure we copy passed in span data/tags/attributes (#10105)

89cef6b

So we do not mutate this if we update data there. Noticed this here: #10097

Uh oh!

Conversation

mydea commented Jan 8, 2024

Uh oh!

github-actions bot commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

lforst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mydea commented Jan 8, 2024

Uh oh!

lforst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Jan 8, 2024 •

edited

Loading