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

Tracing without Performance #2788

Merged
merged 30 commits into from
Jun 30, 2023
Merged

Tracing without Performance #2788

merged 30 commits into from
Jun 30, 2023

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Jun 14, 2023

📜 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
Copy link
Member Author

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

});
}

public static void traceIfAllowed(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then returning TracingHeaders looks already better.

@marandaneto
Copy link
Contributor

@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
Copy link
Contributor

marandaneto commented Jun 16, 2023

@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
Copy link
Member Author

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
Copy link
Member

romtsn commented Jun 16, 2023

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
Copy link
Contributor

marandaneto commented Jun 16, 2023

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
Copy link
Contributor

github-actions bot commented Jun 19, 2023

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

Generated by 🚫 dangerJS against a360064

@github-actions
Copy link
Contributor

github-actions bot commented Jun 19, 2023

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
Copy link

codecov bot commented Jun 20, 2023

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.

Copy link
Member Author

@adinauer adinauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@markushi markushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@adinauer adinauer marked this pull request as ready for review June 20, 2023 12:57
adinauer and others added 3 commits June 26, 2023 17:45
Co-authored-by: Manoel Aranda Neto <5731772+marandaneto@users.noreply.github.com>
@adinauer
Copy link
Member Author

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

@marandaneto
Copy link
Contributor

@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.

CHANGELOG.md Outdated Show resolved Hide resolved
@romtsn
Copy link
Member

romtsn commented Jun 29, 2023

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

Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
Sign up for free to 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.

[java] Tracing without Performance
5 participants