Skip to content
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): Add getTraceData function #13134

Merged
merged 9 commits into from
Aug 2, 2024

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Jul 31, 2024

This PR adds a getTraceData function to the @sentry/core package and re-exports it in none-browser SDKs inheriting from it.

The logic was taken from the getTraceMetaTagValues function in the Astro SDK but I made some changes to its return type and, after feedback, generalized it to be used whenever we need to propagate trace data in our instrumentation.
Therefore, we simply return an object where the keys denote the trace data name (sentry-trace or baggage) and the values the respective content.

This function is added for two reasons

  1. We want to follow this function's logic to obtain the meta tag data in all our meta framework SDKs (Inject meta tags in Tracing-without-Performance-mode  #8520)
  2. It contributes to a request made a long time ago in Add getTracingMetaTags helper function to Node SDK #8843. In a follow-up PR, I'm gonna add a second function that renders stringified HTML tags directly for ease of use.

ref #8843
ref #8520

@Lms24 Lms24 self-assigned this Jul 31, 2024
Copy link
Contributor

github-actions bot commented Jul 31, 2024

size-limit report 📦

Path Size
@sentry/browser 22.46 KB (0%)
@sentry/browser (incl. Tracing) 34.24 KB (0%)
@sentry/browser (incl. Tracing, Replay) 70.3 KB (0%)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 63.63 KB (0%)
@sentry/browser (incl. Tracing, Replay with Canvas) 74.7 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) 87.28 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 89.12 KB (0%)
@sentry/browser (incl. metrics) 26.77 KB (0%)
@sentry/browser (incl. Feedback) 39.38 KB (0%)
@sentry/browser (incl. sendFeedback) 27.08 KB (0%)
@sentry/browser (incl. FeedbackAsync) 31.72 KB (0%)
@sentry/react 25.23 KB (0%)
@sentry/react (incl. Tracing) 37.24 KB (0%)
@sentry/vue 26.61 KB (0%)
@sentry/vue (incl. Tracing) 36.07 KB (0%)
@sentry/svelte 22.59 KB (0%)
CDN Bundle 23.65 KB (0%)
CDN Bundle (incl. Tracing) 35.89 KB (0%)
CDN Bundle (incl. Tracing, Replay) 70.33 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) 75.59 KB (0%)
CDN Bundle - uncompressed 69.41 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed 106.35 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed 218.2 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 231.03 KB (0%)
@sentry/nextjs (client) 37.09 KB (0%)
@sentry/sveltekit (client) 34.81 KB (0%)
@sentry/node 114.77 KB (0%)
@sentry/node - without tracing 89.33 KB (0%)
@sentry/aws-serverless 98.5 KB (0%)

*
* @param span the currently active span
* @param client the SDK's client
*
* @returns an object with the two serialized <meta> tags
* @returns an object with the two meta tags. The object keys are the name of the meta tag,
* the respective value is the content.
*/
export function getTracingMetaTags(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: Can we think of a different name here? IMHO this sounds to me as if that would return the actual tag elements, but it returns strings. What about e.g. getTraceData or something along these lines...?

Copy link
Member Author

@Lms24 Lms24 Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I chose this return type because returning one (or two) string(s) with rendered HTML is too specific of a return type to cover all use cases (e.g. see #8843 (comment)).
I couldn't really come up with a better return type that's still versatile enough but simpler than the current one to use.

So I guess we give it a more general name then. Just a bit unhappy that it's not as simple as in laravel but at the same time I wanna avoid exposing two functions 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the return type is good (more generic, and easier to port)! We can also use something like getTraceMetaTagValues, for example? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah let's go with that, thx!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: I'd probably just have gone with getTraceData and not try to return meta tag names with them. I feel like it's obvious what they should be, and it's not like you actually read them out when you used them.

Would just return { sentryTrace: string, baggage: string | undefined }.

But feel free to disregard.

@Lms24 Lms24 requested review from mydea, a team and AbhiPrasad and removed request for a team July 31, 2024 13:23
@Lms24 Lms24 changed the title feat(node): Add getTracingMetaTags function feat(node): Add getTraceMetaTagValues function Jul 31, 2024
@Lms24 Lms24 changed the title feat(node): Add getTraceMetaTagValues function feat(node): Add getTraceMetaTagValues function Jul 31, 2024
packages/node/src/utils/meta.ts Outdated Show resolved Hide resolved
packages/node/src/utils/meta.ts Outdated Show resolved Hide resolved
packages/node/test/utils/meta.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @andreiborza, getTraceData feels like a better fit. There is nothing in the implementation or return value of getTracingMetaTags that indicates that it has to do with meta tags. Someone can use this helper to get values that they can serialize across an iframe for example.

Here's how I'd like us to approach this, but please feel free to adjust based what you find during your implementation.

  1. packages/node/src/utils/meta.ts should instead live in @sentry/core and be named getTraceData. This way non-node runtimes also get access to it.
  2. We expose another helper that is similar to the logic in packages/astro/src/server/middleware.ts, that generates meta tags as strings <meta XXX>. This should probably be called getTracingMetaTags (and also live in @sentry/core).

Now we have two workflows for users.

Use generated HTML? Use the meta tag helper and just insert that into your HTML document in the right place. Very easy API surface for users because usage is basically like:

const sentryMetaTags = getTracingMetaTags()

renderHtml(`
  <html>
   <head>
     ${sentryMetaTags}
   </head>
  </html>
`);

Want to get access to the these values to serialize in another way because of your super custom instrumentation setup? no problem just call getTraceData directly and do what you need to do.

Afterwards, we probably want to refactor all of our instrumentation to use getTraceData instead of manually calling generateSentryTraceHeader and dynamicSamplingContextToSentryBaggageHeader everywhere.

@Lms24
Copy link
Member Author

Lms24 commented Aug 1, 2024

Thanks for the review @AbhiPrasad! I like your suggestion. I was a bit hesitant about exposing two functions before but since we can't find a good, universal API for one function it's better to split.

I'll repurpose this PR to add the more universal getTraceData function. And I'll add the easy to use string function in a second PR afterwards.

@Lms24 Lms24 marked this pull request as draft August 2, 2024 09:50
@Lms24 Lms24 changed the title feat(node): Add getTraceMetaTagValues function feat(core): Add getTraceData function Aug 2, 2024
@Lms24 Lms24 force-pushed the lms/feat-node-add-getTracingMetaTags branch from 219b070 to 324b747 Compare August 2, 2024 10:20
@Lms24 Lms24 marked this pull request as ready for review August 2, 2024 10:24
@Lms24
Copy link
Member Author

Lms24 commented Aug 2, 2024

I reworked the PR. Would appreciate another round of reviews 🙏

packages/core/src/utils/traceData.ts Outdated Show resolved Hide resolved
packages/core/src/utils/traceData.ts Outdated Show resolved Hide resolved
packages/core/src/utils/traceData.ts Outdated Show resolved Hide resolved
packages/core/src/utils/traceData.ts Outdated Show resolved Hide resolved
packages/core/src/utils/traceData.ts Outdated Show resolved Hide resolved
packages/core/test/lib/utils/traceData.test.ts Outdated Show resolved Hide resolved
packages/core/test/lib/utils/traceData.test.ts Outdated Show resolved Hide resolved
packages/core/test/lib/utils/traceData.test.ts Outdated Show resolved Hide resolved
packages/core/test/lib/utils/traceData.test.ts Outdated Show resolved Hide resolved
packages/core/test/lib/utils/traceData.test.ts Outdated Show resolved Hide resolved
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants