-
-
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): Add getTraceData
function
#13134
Conversation
size-limit report 📦
|
packages/node/src/utils/meta.ts
Outdated
* | ||
* @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( |
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: 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...?
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.
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 😅
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 think the return type is good (more generic, and easier to port)! We can also use something like getTraceMetaTagValues
, for example? 🤔
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 let's go with that, thx!
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.
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.
getTracingMetaTags
functiongetTraceMetaTagValues
function
getTraceMetaTagValues
functiongetTraceMetaTagValues
function
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 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.
packages/node/src/utils/meta.ts
should instead live in@sentry/core
and be namedgetTraceData
. This way non-node runtimes also get access to it.- 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 calledgetTracingMetaTags
(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.
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 |
getTraceMetaTagValues
functiongetTraceData
function
219b070
to
324b747
Compare
I reworked the PR. Would appreciate another round of reviews 🙏 |
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
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
orbaggage
) and the values the respective content.This function is added for two reasons
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