Tracing without Performance

[adinauer]

📜 Description

Pass through / add sentry-trace and baggage even if performance is disabled.

💡 Motivation and Context

Closes #2768

💚 How did you test it?

wanna do some more e2e testing before merging

📝 Checklist

• I reviewed the submitted code.
• I added tests to verify the changes.
• No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
• I updated the docs if needed.
• Review from the native team if needed.
• No breaking change or entry added to the changelog.
• No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

[adinauer]

@markushi and @marandaneto I'll leave some markers where I'd like your input. A full review would be very welcome as well.

[marandaneto]

M: I believe it'd be better if this just returns a boolean?
Executing a callback forces you to have Atomic operations in a few different places.

[adinauer]

Hmm could change it to return the TracingHeaders instead of having a callback. With @Nullable it'd only be a simple null check on the calling side. If this returned a boolean we'd have to duplicate the extraction code everywhere.

[marandaneto]

Then returning TracingHeaders looks already better.

[marandaneto]

@markushi and @marandaneto I'll leave some markers where I'd like your input. A full review would be very welcome as well.

maybe @romtsn or @stefanosiano ? Markus is OOO.

[marandaneto]

@adinauer related to your questions/markers, I believe @cleptric can help? since it's not Android related, but rather how this feature should work, Also https://www.notion.so/sentry/Tracing-without-performance-efab307eb7f64e71a04f09dc72722530

The places you added a TODO are where a new transaction would be created (but performance is disabled), so in this case, you do nothing, right?
The important part is that if there's an outgoing request or something, you have to attach the propagation even if performance is disabled, and the propagation is already living in the Hub/Scope, IIRC.

[adinauer]

It's basically the following question and answer from #2768:

There's other places where we start transactions that don't originate from incoming requests but some user interaction / system call on Android. Do we create a new PropagationContext in all those spots where we'd create a transaction if performance was enabled?
Answer: If the button click (or similar) creates a new TraceID today when using performance, it should also create a new TraceID when performance is off, so a new PropagationContext

So in theory we should be starting a new trace there. I just wanted your input for Android specifics.

[romtsn]

With my (limited) understanding of tracing, I'd say we should start a new trace for all of the markers you left. When UI is involved, a User Interaction (whether navigation or click) usually marks the start of a new trace (e.g. you clicked "Save" button - then you make api/save request, you clicked Delete button - then you make api/delete request; it makes sense that these are two separate traces with two separate span hierarchies).

[marandaneto]

With my (limited) understanding of tracing, I'd say we should start a new trace for all of the markers you left. When UI is involved, a User Interaction (whether navigation or click) usually marks the start of a new trace (e.g. you clicked "Save" button - then you make api/save request, you clicked Delete button - then you make api/delete request; it makes sense that these are two separate traces with two separate span hierarchies).

Yes, exactly, what I meant by "you do nothing because performance is disabled" means, you don't start a transaction.
But you have to reset(or create?) the PropagationContext in the Hub/Scope.
I understood previously that when you create a Hub/Scope, the PropagationContext would be created automatically already, and you do nothing, but the use case above makes more sense.

[github-actions]

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against a360064

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	340.02 ms	429.10 ms	89.08 ms
Size	1.72 MiB	2.29 MiB	574.41 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
dc67004	273.86 ms	346.37 ms	72.51 ms
9246ed4	275.63 ms	321.31 ms	45.69 ms
8820c5c	330.60 ms	416.86 ms	86.26 ms
9246ed4	281.79 ms	352.08 ms	70.29 ms
0310da5	381.20 ms	404.50 ms	23.30 ms
496bdfd	272.86 ms	407.33 ms	134.48 ms
496bdfd	301.22 ms	343.96 ms	42.73 ms

App size

Revision	Plain	With Sentry	Diff
dc67004	1.72 MiB	2.28 MiB	573.45 KiB
9246ed4	1.72 MiB	2.28 MiB	572.22 KiB
8820c5c	1.72 MiB	2.28 MiB	571.82 KiB
9246ed4	1.72 MiB	2.28 MiB	572.22 KiB
0310da5	1.72 MiB	2.28 MiB	573.45 KiB
496bdfd	1.72 MiB	2.28 MiB	571.82 KiB
496bdfd	1.72 MiB	2.28 MiB	571.82 KiB

Previous results on branch: feat/tracing-without-performance

Startup times

Revision	Plain	With Sentry	Diff
25cb71a	322.37 ms	395.86 ms	73.49 ms
d50b334	299.84 ms	401.56 ms	101.72 ms
0a8926a	293.88 ms	333.60 ms	39.72 ms
bfaa13c	256.71 ms	313.62 ms	56.91 ms
c4c8632	295.86 ms	322.98 ms	27.12 ms
9c8777f	280.52 ms	370.46 ms	89.94 ms
03de2a6	281.98 ms	318.64 ms	36.66 ms
1d85b7f	303.18 ms	356.62 ms	53.44 ms
6da169c	295.60 ms	390.86 ms	95.26 ms
8cfda4c	355.00 ms	425.33 ms	70.33 ms

App size

Revision	Plain	With Sentry	Diff
25cb71a	1.72 MiB	2.28 MiB	572.59 KiB
d50b334	1.72 MiB	2.29 MiB	574.13 KiB
0a8926a	1.72 MiB	2.29 MiB	574.47 KiB
bfaa13c	1.72 MiB	2.29 MiB	574.47 KiB
c4c8632	1.72 MiB	2.29 MiB	574.15 KiB
9c8777f	1.72 MiB	2.29 MiB	574.16 KiB
03de2a6	1.72 MiB	2.29 MiB	574.47 KiB
1d85b7f	1.72 MiB	2.28 MiB	572.59 KiB
6da169c	1.72 MiB	2.29 MiB	574.39 KiB
8cfda4c	1.72 MiB	2.29 MiB	574.47 KiB

[codecov]

Codecov Report

Patch coverage: 77.75% and project coverage change: +0.10 🎉

Comparison is base (dc67004) 81.15% compared to head (a360064) 81.25%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2788      +/-   ##
============================================
+ Coverage     81.15%   81.25%   +0.10%     
- Complexity     4501     4560      +59     
============================================
  Files           348      350       +2     
  Lines         16685    16870     +185     
  Branches       2268     2274       +6     
============================================
+ Hits          13540    13708     +168     
- Misses         2198     2219      +21     
+ Partials        947      943       -4     

Impacted Files	Coverage Δ
...y/spring/boot/jakarta/SentryAutoConfiguration.java	97.18% <ø> (ø)
...io/sentry/spring/boot/SentryAutoConfiguration.java	97.18% <ø> (ø)
...racing/SentrySpanClientHttpRequestInterceptor.java	0.00% <0.00%> (ø)
...racing/SentrySpanClientHttpRequestInterceptor.java	0.00% <0.00%> (ø)
sentry/src/main/java/io/sentry/HubAdapter.java	95.58% <0.00%> (-4.42%)	⬇️
sentry/src/main/java/io/sentry/IHub.java	92.59% <ø> (ø)
sentry/src/main/java/io/sentry/NoOpHub.java	44.68% <0.00%> (-3.05%)	⬇️
sentry/src/main/java/io/sentry/Sentry.java	82.75% <0.00%> (-1.09%)	⬇️
sentry/src/main/java/io/sentry/Hub.java	75.17% <44.44%> (-1.34%)	⬇️
...ry/src/main/java/io/sentry/PropagationContext.java	74.50% <74.50%> (ø)
... and 15 more

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[adinauer]

@marandaneto @romtsn @markushi I've updated the PR now so should be good to review. Not sure about gesture listener and starting a new trace there.

[markushi]

Nice one, already looks pretty complete to me 🚀 Left a few comments.

[adinauer]

@marandaneto any more changes you'd like to see before I merge this?

[marandaneto]

@marandaneto any more changes you'd like to see before I merge this?

PR is already approved, and all my comments were addressed, all good, thank you.

[romtsn]

Made a couple of more suggestions as I realized we check for the breadcrumbs flag inside addBreadcrumb already, so we don't need it in onActivityResumed etc, but feel free to merge after that is addressed