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

Add automatic trace support for Spring WebClient #1621

Merged
merged 7 commits into from
Aug 2, 2021

Conversation

yhware
Copy link
Contributor

@yhware yhware commented Jul 25, 2021

📜 Description

When using WebClient in Spring, automatically add span and breadcrumb for tracing.
There's already support for RestTemplate (https://docs.sentry.io/platforms/java/guides/spring-boot/performance/instrumentation/automatic-instrumentation/)

💡 Motivation and Context

Resolves: Support for Spring WebClient for creating spans/crumbs #1619

💚 How did you test it?

Ported all tests that were available for RestTemplate integration.
Also ran the sample spring boot application with my personal DSN and verified it worked as intended.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

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.

Looks great! @maciejwalkowiak might have some thoughts as the author of the RestTemplate bits

@marandaneto
Copy link
Contributor

looks good, I'll let @maciejwalkowiak review though since its backend bits.
@yhware thanks for that, would you mind working on the broken CI? this includes running make format to format files, make api to check binary compatibility validator and adding an entry to the changelog.

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

left a comment to fix CI but changes are good LGTM

Copy link
Contributor

@maciejwalkowiak maciejwalkowiak left a comment

Choose a reason for hiding this comment

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

@yhware thanks a lot for this PR! 🤩 Few small things have to be polished to get it merged. Also, make sure to run ./gradlew apiDump to update files containing public APIs.

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2021

Codecov Report

Merging #1621 (662d95f) into main (1c39818) will decrease coverage by 0.30%.
The diff coverage is 18.91%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1621      +/-   ##
============================================
- Coverage     75.93%   75.63%   -0.31%     
- Complexity     2004     2006       +2     
============================================
  Files           202      204       +2     
  Lines          6943     6980      +37     
  Branches        691      692       +1     
============================================
+ Hits           5272     5279       +7     
- Misses         1335     1365      +30     
  Partials        336      336              
Impacted Files Coverage Δ
...ring/tracing/SentrySpanClientWebRequestFilter.java 0.00% <0.00%> (ø)
...io/sentry/spring/boot/SentryAutoConfiguration.java 98.33% <100.00%> (+0.05%) ⬆️
...try/spring/boot/SentrySpanWebClientCustomizer.java 100.00% <100.00%> (ø)

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 1c39818...662d95f. Read the comment docs.

@bruno-garcia bruno-garcia mentioned this pull request Jul 28, 2021
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak left a comment

Choose a reason for hiding this comment

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

Thanks for fixes!

@maciejwalkowiak maciejwalkowiak merged commit bb9f248 into getsentry:main Aug 2, 2021
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.

5 participants