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

Performance: Refactor resolving SpanContext for Throwable #1068

Merged
merged 17 commits into from
Nov 30, 2020

Conversation

maciejwalkowiak
Copy link
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Refactor resolving SpanContext for Throwable

💡 Motivation and Context

Previous implementation would fail once we switch to sending individual spans. Here we keep an association between throwable and SpanContext on the Hub which is independent from transaction or span being still in memory.

💚 How did you test it?

Integration tests cover it.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

@codecov-io
Copy link

codecov-io commented Nov 25, 2020

Codecov Report

Merging #1068 (4d11d9c) into main (bb58d45) will decrease coverage by 0.46%.
The diff coverage is 43.47%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1068      +/-   ##
============================================
- Coverage     73.17%   72.71%   -0.47%     
+ Complexity     1542     1516      -26     
============================================
  Files           162      161       -1     
  Lines          5510     5461      -49     
  Branches        562      548      -14     
============================================
- Hits           4032     3971      -61     
- Misses         1189     1199      +10     
- Partials        289      291       +2     
Impacted Files Coverage Δ Complexity Δ
sentry/src/main/java/io/sentry/HubAdapter.java 8.62% <0.00%> (-0.48%) 4.00 <0.00> (ø)
sentry/src/main/java/io/sentry/IHub.java 80.00% <ø> (ø) 8.00 <0.00> (ø)
sentry/src/main/java/io/sentry/Hub.java 63.89% <14.28%> (-0.84%) 69.00 <0.00> (ø)
...try/src/main/java/io/sentry/SentryTransaction.java 94.00% <33.33%> (+1.01%) 23.00 <0.00> (-3.00) ⬆️
...java/io/sentry/spring/SentryExceptionResolver.java 88.88% <50.00%> (+12.88%) 3.00 <0.00> (-2.00) ⬆️
sentry/src/main/java/io/sentry/NoOpHub.java 58.33% <100.00%> (+5.39%) 21.00 <2.00> (+3.00)
sentry/src/main/java/io/sentry/Span.java 88.88% <100.00%> (-4.45%) 10.00 <2.00> (ø)
...sentry/spring/tracing/TransactionNameProvider.java 66.66% <0.00%> (-33.34%) 2.00% <0.00%> (-1.00%)
...entry/src/main/java/io/sentry/SentryBaseEvent.java 72.72% <0.00%> (-18.19%) 14.00% <0.00%> (-4.00%)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb58d45...d8f984c. Read the comment docs.

@maciejwalkowiak
Copy link
Contributor Author

@bruno-garcia @marandaneto please take a look.

@maciejwalkowiak
Copy link
Contributor Author

As far as I can tell covecov reports are broken. Since we have a check for code coverage in gradle build already, could we turn it off?

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

🚀

sentry/src/main/java/io/sentry/HubAdapter.java Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/IHub.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/NoOpHub.java Outdated Show resolved Hide resolved
@maciejwalkowiak maciejwalkowiak merged commit 2e3679d into main Nov 30, 2020
@maciejwalkowiak maciejwalkowiak deleted the refactor-throwable-to-span-context branch November 30, 2020 12:13
This pull request was closed.
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.

4 participants