feat(tracing): Add `tracePropagationTargets` option for adding `sentry-trace` header #6041

timfish · 2022-10-25T15:41:21Z

Adds tracePropagationTargets option with same default value as tracingOrigins
Adds headers if and only if there's a match to either tracingOrigins or tracePropagationTargets
- Both callbacks already bail early if creating spans is disabled so there are no headers if there is no span

Deprecation of tracingOrigins and adjusting of how logging for missing args works will come in the next PR...

…dding `sentry-trace` headers

lobsterkatie

Looks good!

Re: my feedback on tests below, I realize some of it a little out of scope for this PR, and probably some or all of the changes actually belong in the "fix tracingOrigins" PR, but the addition of passing shouldAttachHeaders here makes them easy to do. Depends on how much we want to be purists about keeping changes strictly in their relevant PRs.

I think maybe let's hold on this question until we decide for sure if we want to go through with that other change now or wait until v8. Also, if we do do them here, let's make the same changes to the XHR tests. Updated my comment on the original summary after seeing Abhi's comment on the other PR. I think we have critical mass. Let's make that change, and I'll leave it to you to figure out which PR makes more sense to include the tests I'm talking about.

lobsterkatie · 2022-10-26T23:02:05Z

packages/tracing/src/browser/request.ts

@@ -99,12 +106,12 @@ export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions
  traceFetch: true,
  traceXHR: true,
  tracingOrigins: DEFAULT_TRACING_ORIGINS,
+  tracePropagationTargets: DEFAULT_TRACING_ORIGINS,


For simplicity when it comes time to delete tracingOrigins, can we give this its own DEFAULT_TRACE_PROPAGATION_TARGETS variable? That way, we won't have to change this line when we do that.

lobsterkatie · 2022-10-26T23:02:43Z

packages/tracing/test/browser/request.test.ts

+  const always = () => true;
+  const never = () => false;


Suggested change

const always = () => true;

const never = () => false;

const alwaysCreateSpan = () => true;

const neverCreateSpan = () => false;

const alwaysAttachHeaders = () => true;

const neverAttachHeaders = () => false;

I get that to a certain extent this is redundant, but then it allows your tests below to look like

fetchCallback(fetchHandlerData, alwaysCreateSpan, neverAttachHeaders, spans)

rather than

fetchCallback(fetchHandlerData, always, never, spans)

which is significantly more self-documenting.

lobsterkatie · 2022-10-26T23:03:26Z

packages/tracing/test/browser/request.test.ts

@@ -87,7 +87,7 @@ describe('callbacks', () => {
    it('does not create span if shouldCreateSpan returns false', () => {


Can we combine this and the 'adds sentry-trace header to fetch requests' test below into a group of four tests testing the four combinations of always and never for both shouldCreateSpan and shouldAttachHeaders, each testing both span creation and header addition?

lobsterkatie · 2022-10-26T23:03:29Z

packages/tracing/test/browser/request.test.ts

@@ -96,15 +96,15 @@ describe('callbacks', () => {
      const noFetchData = { args: fetchHandlerData.args, startTimestamp: fetchHandlerData.startTimestamp };


It won't let me put this on the line above with the actual test name for some reason, but can this become a 'neither creates span nor adds headers if there is no fetch data in handler data' test, using always, always rather than always, never?

lobsterkatie · 2022-10-26T23:03:35Z

packages/tracing/test/browser/request.test.ts

@@ -96,15 +96,15 @@ describe('callbacks', () => {
      const noFetchData = { args: fetchHandlerData.args, startTimestamp: fetchHandlerData.startTimestamp };
      const spans = {};

-      fetchCallback(noFetchData, alwaysCreateSpan, spans);
+      fetchCallback(noFetchData, always, never, spans);
      expect(spans).toEqual({});
    });

    it('does not add fetch request spans if tracing is disabled', () => {


Similarly, we could combine this test and the 'does not add fetch request headers if tracing is disabled' test below into a 'neither creates span nor adds headers if tracing is disabled' test. And again similarly, I think it should be always, always rather than always, never, to show that tracing being disabled takes precedence over both of the other options.

lobsterkatie · 2022-10-27T00:36:16Z

packages/tracing/src/browser/request.ts

+  const shouldAttachHeaders = (url: string): boolean =>
+    tracingOrigins.some(origin => isMatchingPattern(url, origin)) ||
+    tracePropagationTargets.some(origin => isMatchingPattern(url, origin));


I'm torn between loving the simplicity of this implementation and thinking we should include the kind of result-caching which is being pulled out of shouldCreateSpan in #6039.

Realistically, in a modern browser on modern hardware, maybe it doesn't make enough of a difference to be worth the extra bytes. Opinions, @getsentry/team-web-sdk-frontend?

as the person who put the cache setup there, it was a bit of a premature optimization.

I think it shouldn’t be that hard to add it back, so let’s just proceed with this.

lobsterkatie · 2022-10-28T03:23:53Z

Given #6077, I'm going to close this in favor of #6079 (the part of this that fixes that bug) and #6080 (the part that actually adds tracePropagationTargets).

In the process of working on #5285, we missed the fact that the first two PRs (#6039 and #6041) were interdependent, in that the former accidentally introduced a bug (#6077) which the latter then inadvertently fixed. This would have been fine, except that we published a release after merging the bug-creating PR but before merging the bug-fixing PR. Whoops. This patch pulls just the bug-fixing part out of the second PR. It also adds tests to cover the buggy cases, using `it.each` to cover all of the different combinations of outcomes for `shouldCreateSpanForRequest` and `shouldAttachHeaders`. Finally, since I was already in the test file, I reorganized it a little: - `it('does not create span if shouldCreateSpan returns false')` -> absorbed into the `it.each()` - `it('does not create span if there is no fetch data in handler data')` -> added header check, became `it('adds neither fetch request spans nor fetch request headers if there is no fetch data in handler data')` - `it('does not add fetch request spans if tracing is disabled')` and `it('does not add fetch request headers if tracing is disabled` -> combined into `it('adds neither fetch request spans nor fetch request headers if tracing is disabled')` - `it('adds sentry-trace header to fetch requests')` -> absorbed into the `it.each()` - Similar changes made to XHR tests Co-authored-by: Tim Fish <tim@timfish.uk>

@timfish

…g instrumentation (#6080) As part of the work on #5285, this adds a new option, `tracePropagationTargets`, to our browser-side tracing instrumentation, to live alongside (and eventually take the place of) `tracingOrigins`. Its behavior matches that of `tracingOrigins`, but it has a much clearer name. Switching from internal use of `tracingOrigins` to `tracePropagationTargets` to come in future PRs. Note: This is what remained of #6041 after the fix in #6079 was split off. h/t @timfish for the initial work in that PR.

Add tracePropagationTargets and combine with tracingOrigins for a…

f8a01c9

…dding `sentry-trace` headers

timfish mentioned this pull request Oct 25, 2022

[tracing] Separate span creation from header propagation #5285

Closed

9 tasks

timfish self-assigned this Oct 25, 2022

Fix tests

fbdb1a4

timfish marked this pull request as ready for review October 25, 2022 16:57

lobsterkatie reviewed Oct 26, 2022

View reviewed changes

lobsterkatie reviewed Oct 27, 2022

View reviewed changes

lobsterkatie closed this Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tracing): Add `tracePropagationTargets` option for adding `sentry-trace` header #6041

feat(tracing): Add `tracePropagationTargets` option for adding `sentry-trace` header #6041

timfish commented Oct 25, 2022

lobsterkatie left a comment •

edited

lobsterkatie Oct 26, 2022

lobsterkatie Oct 26, 2022

lobsterkatie Oct 26, 2022

lobsterkatie Oct 26, 2022

lobsterkatie Oct 26, 2022

lobsterkatie Oct 27, 2022

AbhiPrasad Oct 27, 2022

lobsterkatie commented Oct 28, 2022

-  const always = () => true;
-  const never = () => false;
+  const alwaysCreateSpan = () => true;
+  const neverCreateSpan = () => false;
+  const alwaysAttachHeaders = () => true;
+  const neverAttachHeaders = () => false;

		@@ -87,7 +87,7 @@ describe('callbacks', () => {
		it('does not create span if shouldCreateSpan returns false', () => {

		@@ -96,15 +96,15 @@ describe('callbacks', () => {
		const noFetchData = { args: fetchHandlerData.args, startTimestamp: fetchHandlerData.startTimestamp };

feat(tracing): Add tracePropagationTargets option for adding sentry-trace header #6041

feat(tracing): Add tracePropagationTargets option for adding sentry-trace header #6041

Conversation

timfish commented Oct 25, 2022

lobsterkatie left a comment • edited

Choose a reason for hiding this comment

lobsterkatie Oct 26, 2022

Choose a reason for hiding this comment

lobsterkatie Oct 26, 2022

Choose a reason for hiding this comment

lobsterkatie Oct 26, 2022

Choose a reason for hiding this comment

lobsterkatie Oct 26, 2022

Choose a reason for hiding this comment

lobsterkatie Oct 26, 2022

Choose a reason for hiding this comment

lobsterkatie Oct 27, 2022

Choose a reason for hiding this comment

AbhiPrasad Oct 27, 2022

Choose a reason for hiding this comment

lobsterkatie commented Oct 28, 2022

feat(tracing): Add `tracePropagationTargets` option for adding `sentry-trace` header #6041

feat(tracing): Add `tracePropagationTargets` option for adding `sentry-trace` header #6041

lobsterkatie left a comment •

edited