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: Pick up sentry-trace in JS <meta/> tag #2703

Merged
merged 3 commits into from Jun 30, 2020
Merged

Conversation

@HazAT
Copy link
Member

@HazAT HazAT commented Jun 29, 2020

Add functionality to Tracing integration to pick up <meta name="sentry-trace" /> to continue a trace in the frontend.

The reason why it's a <meta/> tag is that a <script/> tag setting something on the window would block rendering.

Right now, only Laravel can provide this meta tag with this PR
getsentry/sentry-laravel#358

This is for cases where the Trace is not started from the frontend but from the backend.
It can be also useful for other webframeworks that are not SPA and instead still render templates from backend -> frontend.

@HazAT HazAT requested a review from kamilogorek as a code owner Jun 29, 2020
@HazAT HazAT self-assigned this Jun 29, 2020
@HazAT HazAT changed the title feat: Pick up sentry-trace in frontend feat: Pick up sentry-trace in JS <meta/> tag Jun 29, 2020
@getsentry-bot
Copy link

@getsentry-bot getsentry-bot commented Jun 29, 2020

Messages
📖

@sentry/browser bundle gzip'ed minified size: (ES5: 17.1377 kB) (ES6: 16.251 kB)

📖 TSLint passed

Generated by 🚫 dangerJS against cb0494e

let parentSpanId;
let sampled;

const header = Tracing._getMeta('sentry-trace');

This comment has been minimized.

@AbhiPrasad

AbhiPrasad Jun 29, 2020
Contributor

So I guess now if someone sets this in an SPA, all transactions would use this meta header right?

This comment has been minimized.

@HazAT

HazAT Jun 29, 2020
Author Member

Correct. For me this is opt-in behavior enough, this can't happen by accident so guarding this behind an option in the integration doesn't make a lot of sense to me.

Copy link
Contributor

@AbhiPrasad AbhiPrasad left a comment

:shipit:

@@ -42,13 +43,15 @@ export function tracingHandler(): (
if (span) {
traceId = span.traceId;
parentSpanId = span.parentSpanId;
sampled = span.sampled;

This comment has been minimized.

@AbhiPrasad

AbhiPrasad Jun 29, 2020
Contributor

This is a bug fix right? We should remember for the changelog then that there are two changes in this PR. a feat for the meta tag, and a fix for sampling from trace parent in node.

private static _getMeta(metaName: string): string | null {
const metas = document.getElementsByTagName('meta');

// tslint:disable-next-line: prefer-for-of
for (let i = 0; i < metas.length; i++) {
if (metas[i].getAttribute('name') === metaName) {
return metas[i].getAttribute('content');
}
}

return null;
}
Comment on lines 319 to 330

This comment has been minimized.

@kamilogorek

kamilogorek Jun 30, 2020
Member

Suggested change
private static _getMeta(metaName: string): string | null {
const metas = document.getElementsByTagName('meta');
// tslint:disable-next-line: prefer-for-of
for (let i = 0; i < metas.length; i++) {
if (metas[i].getAttribute('name') === metaName) {
return metas[i].getAttribute('content');
}
}
return null;
}
private static _getMeta(metaName: string): string | undefined {
return (document.querySelector(`meta[name=${metaName}]`) || {}).content;
}

This comment has been minimized.

@HazAT

HazAT Jun 30, 2020
Author Member

This has typescript errors, I will go with

const el = document.querySelector(`meta[name=${metaName}]`);
return el ? el.getAttribute('content') : null;

This comment has been minimized.

@kamilogorek

kamilogorek Jun 30, 2020
Member

Ah, silly empty object typing 🎳

parentSpanId = span.parentSpanId;
sampled = span.sampled;
}
Tracing._log(

This comment has been minimized.

@kamilogorek

kamilogorek Jun 30, 2020
Member

This log will be incorrect if span is falsy. Should be moved inside span or corrected logically.

@HazAT HazAT merged commit a551313 into master Jun 30, 2020
1 of 2 checks passed
1 of 2 checks passed
Travis CI - Pull Request Build Errored
Details
Danger All green. Woo!
Details
@HazAT HazAT deleted the feat/sentry-trace-meta branch Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.