feat(core): Add getTraceData function - #13134

Merged
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags
Aug 2, 2024
Merged

feat(core): Add getTraceData function#13134
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags

Conversation

@Lms24

@Lms24Lms24 commented Jul 31, 2024

Copy link
Copy Markdown
Member

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

@Lms24Lms24 self-assigned this Jul 31, 2024
@github-actions

github-actionsBot commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser22.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 flags63.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/react25.23 KB (0%)
@sentry/react (incl. Tracing)37.24 KB (0%)
@sentry/vue26.61 KB (0%)
@sentry/vue (incl. Tracing)36.07 KB (0%)
@sentry/svelte22.59 KB (0%)
CDN Bundle23.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 - uncompressed69.41 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed106.35 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed218.2 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed231.03 KB (0%)
@sentry/nextjs (client)37.09 KB (0%)
@sentry/sveltekit (client)34.81 KB (0%)
@sentry/node114.77 KB (0%)
@sentry/node - without tracing89.33 KB (0%)
@sentry/aws-serverless98.5 KB (0%)

Comment threadpackages/astro/src/server/middleware.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
* @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
Copy Markdown
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...?

@Lms24Lms24Jul 31, 2024

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
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 a team, AbhiPrasad and mydea and removed request for a teamJuly 31, 2024 13:23
@Lms24Lms24 changed the title feat(node): Add getTracingMetaTags functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/test/utils/meta.test.ts Outdated

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

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:

constsentryMetaTags=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

Lms24 commented Aug 1, 2024

Copy link
Copy Markdown
MemberAuthor

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
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(core): Add getTraceData functionAug 2, 2024
@Lms24
Lms24force-pushed the lms/feat-node-add-getTracingMetaTags branch from 219b070 to 324b747CompareAugust 2, 2024 10:20
@Lms24
Lms24 marked this pull request as ready for review August 2, 2024 10:24
@Lms24

Lms24 commented Aug 2, 2024

Copy link
Copy Markdown
MemberAuthor

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

Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
Sign up for freeto 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

@Lms24@mydea@AbhiPrasad@andreiborza
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

feat(core): Add getTraceData function - #13134

Merged
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags
Aug 2, 2024
Merged

feat(core): Add getTraceData function#13134
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags

Conversation

@Lms24

@Lms24Lms24 commented Jul 31, 2024

Copy link
Copy Markdown
Member

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

@Lms24Lms24 self-assigned this Jul 31, 2024
@github-actions

github-actionsBot commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser22.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 flags63.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/react25.23 KB (0%)
@sentry/react (incl. Tracing)37.24 KB (0%)
@sentry/vue26.61 KB (0%)
@sentry/vue (incl. Tracing)36.07 KB (0%)
@sentry/svelte22.59 KB (0%)
CDN Bundle23.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 - uncompressed69.41 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed106.35 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed218.2 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed231.03 KB (0%)
@sentry/nextjs (client)37.09 KB (0%)
@sentry/sveltekit (client)34.81 KB (0%)
@sentry/node114.77 KB (0%)
@sentry/node - without tracing89.33 KB (0%)
@sentry/aws-serverless98.5 KB (0%)

Comment threadpackages/astro/src/server/middleware.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
* @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
Copy Markdown
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...?

@Lms24Lms24Jul 31, 2024

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
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 a team, AbhiPrasad and mydea and removed request for a teamJuly 31, 2024 13:23
@Lms24Lms24 changed the title feat(node): Add getTracingMetaTags functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/test/utils/meta.test.ts Outdated

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

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:

constsentryMetaTags=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

Lms24 commented Aug 1, 2024

Copy link
Copy Markdown
MemberAuthor

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
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(core): Add getTraceData functionAug 2, 2024
@Lms24
Lms24force-pushed the lms/feat-node-add-getTracingMetaTags branch from 219b070 to 324b747CompareAugust 2, 2024 10:20
@Lms24
Lms24 marked this pull request as ready for review August 2, 2024 10:24
@Lms24

Lms24 commented Aug 2, 2024

Copy link
Copy Markdown
MemberAuthor

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

Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
Sign up for freeto 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

@Lms24@mydea@AbhiPrasad@andreiborza
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(core): Add getTraceData function - #13134

Merged
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags
Aug 2, 2024
Merged

feat(core): Add getTraceData function#13134
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags

Conversation

@Lms24

@Lms24Lms24 commented Jul 31, 2024

Copy link
Copy Markdown
Member

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

@Lms24Lms24 self-assigned this Jul 31, 2024
@github-actions

github-actionsBot commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser22.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 flags63.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/react25.23 KB (0%)
@sentry/react (incl. Tracing)37.24 KB (0%)
@sentry/vue26.61 KB (0%)
@sentry/vue (incl. Tracing)36.07 KB (0%)
@sentry/svelte22.59 KB (0%)
CDN Bundle23.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 - uncompressed69.41 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed106.35 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed218.2 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed231.03 KB (0%)
@sentry/nextjs (client)37.09 KB (0%)
@sentry/sveltekit (client)34.81 KB (0%)
@sentry/node114.77 KB (0%)
@sentry/node - without tracing89.33 KB (0%)
@sentry/aws-serverless98.5 KB (0%)

Comment threadpackages/astro/src/server/middleware.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
* @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
Copy Markdown
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...?

@Lms24Lms24Jul 31, 2024

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
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 a team, AbhiPrasad and mydea and removed request for a teamJuly 31, 2024 13:23
@Lms24Lms24 changed the title feat(node): Add getTracingMetaTags functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/test/utils/meta.test.ts Outdated

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

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:

constsentryMetaTags=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

Lms24 commented Aug 1, 2024

Copy link
Copy Markdown
MemberAuthor

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
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(core): Add getTraceData functionAug 2, 2024
@Lms24
Lms24force-pushed the lms/feat-node-add-getTracingMetaTags branch from 219b070 to 324b747CompareAugust 2, 2024 10:20
@Lms24
Lms24 marked this pull request as ready for review August 2, 2024 10:24
@Lms24

Lms24 commented Aug 2, 2024

Copy link
Copy Markdown
MemberAuthor

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

Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
Sign up for freeto 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

@Lms24@mydea@AbhiPrasad@andreiborza
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(core): Add getTraceData function - #13134

Merged
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags
Aug 2, 2024
Merged

feat(core): Add getTraceData function#13134
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags

Conversation

@Lms24

@Lms24Lms24 commented Jul 31, 2024

Copy link
Copy Markdown
Member

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

@Lms24Lms24 self-assigned this Jul 31, 2024
@github-actions

github-actionsBot commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser22.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 flags63.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/react25.23 KB (0%)
@sentry/react (incl. Tracing)37.24 KB (0%)
@sentry/vue26.61 KB (0%)
@sentry/vue (incl. Tracing)36.07 KB (0%)
@sentry/svelte22.59 KB (0%)
CDN Bundle23.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 - uncompressed69.41 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed106.35 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed218.2 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed231.03 KB (0%)
@sentry/nextjs (client)37.09 KB (0%)
@sentry/sveltekit (client)34.81 KB (0%)
@sentry/node114.77 KB (0%)
@sentry/node - without tracing89.33 KB (0%)
@sentry/aws-serverless98.5 KB (0%)

Comment threadpackages/astro/src/server/middleware.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
* @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
Copy Markdown
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...?

@Lms24Lms24Jul 31, 2024

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
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 a team, AbhiPrasad and mydea and removed request for a teamJuly 31, 2024 13:23
@Lms24Lms24 changed the title feat(node): Add getTracingMetaTags functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/test/utils/meta.test.ts Outdated

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

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:

constsentryMetaTags=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

Lms24 commented Aug 1, 2024

Copy link
Copy Markdown
MemberAuthor

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
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(core): Add getTraceData functionAug 2, 2024
@Lms24
Lms24force-pushed the lms/feat-node-add-getTracingMetaTags branch from 219b070 to 324b747CompareAugust 2, 2024 10:20
@Lms24
Lms24 marked this pull request as ready for review August 2, 2024 10:24
@Lms24

Lms24 commented Aug 2, 2024

Copy link
Copy Markdown
MemberAuthor

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

Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
Sign up for freeto 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

@Lms24@mydea@AbhiPrasad@andreiborza
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

feat(core): Add getTraceData function - #13134

Merged
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags
Aug 2, 2024
Merged

feat(core): Add getTraceData function#13134
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags

Conversation

@Lms24

@Lms24Lms24 commented Jul 31, 2024

Copy link
Copy Markdown
Member

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

@Lms24Lms24 self-assigned this Jul 31, 2024
@github-actions

github-actionsBot commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser22.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 flags63.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/react25.23 KB (0%)
@sentry/react (incl. Tracing)37.24 KB (0%)
@sentry/vue26.61 KB (0%)
@sentry/vue (incl. Tracing)36.07 KB (0%)
@sentry/svelte22.59 KB (0%)
CDN Bundle23.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 - uncompressed69.41 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed106.35 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed218.2 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed231.03 KB (0%)
@sentry/nextjs (client)37.09 KB (0%)
@sentry/sveltekit (client)34.81 KB (0%)
@sentry/node114.77 KB (0%)
@sentry/node - without tracing89.33 KB (0%)
@sentry/aws-serverless98.5 KB (0%)

Comment threadpackages/astro/src/server/middleware.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
* @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
Copy Markdown
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...?

@Lms24Lms24Jul 31, 2024

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
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 a team, AbhiPrasad and mydea and removed request for a teamJuly 31, 2024 13:23
@Lms24Lms24 changed the title feat(node): Add getTracingMetaTags functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/test/utils/meta.test.ts Outdated

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

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:

constsentryMetaTags=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

Lms24 commented Aug 1, 2024

Copy link
Copy Markdown
MemberAuthor

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
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(core): Add getTraceData functionAug 2, 2024
@Lms24
Lms24force-pushed the lms/feat-node-add-getTracingMetaTags branch from 219b070 to 324b747CompareAugust 2, 2024 10:20
@Lms24
Lms24 marked this pull request as ready for review August 2, 2024 10:24
@Lms24

Lms24 commented Aug 2, 2024

Copy link
Copy Markdown
MemberAuthor

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

Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
Sign up for freeto 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

@Lms24@mydea@AbhiPrasad@andreiborza
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(core): Add getTraceData function - #13134

Merged
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags
Aug 2, 2024
Merged

feat(core): Add getTraceData function#13134
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags

Conversation

@Lms24

@Lms24Lms24 commented Jul 31, 2024

Copy link
Copy Markdown
Member

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

@Lms24Lms24 self-assigned this Jul 31, 2024
@github-actions

github-actionsBot commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser22.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 flags63.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/react25.23 KB (0%)
@sentry/react (incl. Tracing)37.24 KB (0%)
@sentry/vue26.61 KB (0%)
@sentry/vue (incl. Tracing)36.07 KB (0%)
@sentry/svelte22.59 KB (0%)
CDN Bundle23.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 - uncompressed69.41 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed106.35 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed218.2 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed231.03 KB (0%)
@sentry/nextjs (client)37.09 KB (0%)
@sentry/sveltekit (client)34.81 KB (0%)
@sentry/node114.77 KB (0%)
@sentry/node - without tracing89.33 KB (0%)
@sentry/aws-serverless98.5 KB (0%)

Comment threadpackages/astro/src/server/middleware.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
* @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
Copy Markdown
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...?

@Lms24Lms24Jul 31, 2024

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
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 a team, AbhiPrasad and mydea and removed request for a teamJuly 31, 2024 13:23
@Lms24Lms24 changed the title feat(node): Add getTracingMetaTags functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/test/utils/meta.test.ts Outdated

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

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:

constsentryMetaTags=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

Lms24 commented Aug 1, 2024

Copy link
Copy Markdown
MemberAuthor

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
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(core): Add getTraceData functionAug 2, 2024
@Lms24
Lms24force-pushed the lms/feat-node-add-getTracingMetaTags branch from 219b070 to 324b747CompareAugust 2, 2024 10:20
@Lms24
Lms24 marked this pull request as ready for review August 2, 2024 10:24
@Lms24

Lms24 commented Aug 2, 2024

Copy link
Copy Markdown
MemberAuthor

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

Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
Sign up for freeto 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

@Lms24@mydea@AbhiPrasad@andreiborza
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

feat(core): Add getTraceData function - #13134

Merged
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags
Aug 2, 2024
Merged

feat(core): Add getTraceData function#13134
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags

Conversation

@Lms24

@Lms24Lms24 commented Jul 31, 2024

Copy link
Copy Markdown
Member

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

@Lms24Lms24 self-assigned this Jul 31, 2024
@github-actions

github-actionsBot commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser22.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 flags63.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/react25.23 KB (0%)
@sentry/react (incl. Tracing)37.24 KB (0%)
@sentry/vue26.61 KB (0%)
@sentry/vue (incl. Tracing)36.07 KB (0%)
@sentry/svelte22.59 KB (0%)
CDN Bundle23.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 - uncompressed69.41 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed106.35 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed218.2 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed231.03 KB (0%)
@sentry/nextjs (client)37.09 KB (0%)
@sentry/sveltekit (client)34.81 KB (0%)
@sentry/node114.77 KB (0%)
@sentry/node - without tracing89.33 KB (0%)
@sentry/aws-serverless98.5 KB (0%)

Comment threadpackages/astro/src/server/middleware.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
* @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
Copy Markdown
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...?

@Lms24Lms24Jul 31, 2024

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
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 a team, AbhiPrasad and mydea and removed request for a teamJuly 31, 2024 13:23
@Lms24Lms24 changed the title feat(node): Add getTracingMetaTags functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/test/utils/meta.test.ts Outdated

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

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:

constsentryMetaTags=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

Lms24 commented Aug 1, 2024

Copy link
Copy Markdown
MemberAuthor

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
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(core): Add getTraceData functionAug 2, 2024
@Lms24
Lms24force-pushed the lms/feat-node-add-getTracingMetaTags branch from 219b070 to 324b747CompareAugust 2, 2024 10:20
@Lms24
Lms24 marked this pull request as ready for review August 2, 2024 10:24
@Lms24

Lms24 commented Aug 2, 2024

Copy link
Copy Markdown
MemberAuthor

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

Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
Sign up for freeto 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

@Lms24@mydea@AbhiPrasad@andreiborza
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

feat(core): Add getTraceData function - #13134

Merged
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags
Aug 2, 2024
Merged

feat(core): Add getTraceData function#13134
Lms24 merged 9 commits into
developfrom
lms/feat-node-add-getTracingMetaTags

Conversation

@Lms24

@Lms24Lms24 commented Jul 31, 2024

Copy link
Copy Markdown
Member

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

@Lms24Lms24 self-assigned this Jul 31, 2024
@github-actions

github-actionsBot commented Jul 31, 2024

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
@sentry/browser22.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 flags63.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/react25.23 KB (0%)
@sentry/react (incl. Tracing)37.24 KB (0%)
@sentry/vue26.61 KB (0%)
@sentry/vue (incl. Tracing)36.07 KB (0%)
@sentry/svelte22.59 KB (0%)
CDN Bundle23.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 - uncompressed69.41 KB (0%)
CDN Bundle (incl. Tracing) - uncompressed106.35 KB (0%)
CDN Bundle (incl. Tracing, Replay) - uncompressed218.2 KB (0%)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed231.03 KB (0%)
@sentry/nextjs (client)37.09 KB (0%)
@sentry/sveltekit (client)34.81 KB (0%)
@sentry/node114.77 KB (0%)
@sentry/node - without tracing89.33 KB (0%)
@sentry/aws-serverless98.5 KB (0%)

Comment threadpackages/astro/src/server/middleware.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
* @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
Copy Markdown
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...?

@Lms24Lms24Jul 31, 2024

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Copy Markdown
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 a team, AbhiPrasad and mydea and removed request for a teamJuly 31, 2024 13:23
@Lms24Lms24 changed the title feat(node): Add getTracingMetaTags functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(node): Add getTraceMetaTagValues functionJul 31, 2024
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/src/utils/meta.ts Outdated
Comment threadpackages/node/test/utils/meta.test.ts Outdated

@AbhiPrasadAbhiPrasad left a comment

Copy link
Copy Markdown
Contributor

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:

constsentryMetaTags=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

Lms24 commented Aug 1, 2024

Copy link
Copy Markdown
MemberAuthor

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
@Lms24Lms24 changed the title feat(node): Add getTraceMetaTagValues functionfeat(core): Add getTraceData functionAug 2, 2024
@Lms24
Lms24force-pushed the lms/feat-node-add-getTracingMetaTags branch from 219b070 to 324b747CompareAugust 2, 2024 10:20
@Lms24
Lms24 marked this pull request as ready for review August 2, 2024 10:24
@Lms24

Lms24 commented Aug 2, 2024

Copy link
Copy Markdown
MemberAuthor

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

Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/src/utils/traceData.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Comment threadpackages/core/test/lib/utils/traceData.test.ts Outdated
Co-authored-by: Andrei <168741329+andreiborza@users.noreply.github.com>
Sign up for freeto 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

@Lms24@mydea@AbhiPrasad@andreiborza