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.

Sign up for GitHub

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

Jump to bottom

Support Spring's new RestClient with auto configuration (#3198) #3199

Merged
merged 8 commits into from
Mar 21, 2024
Merged

Support Spring's new RestClient with auto configuration (#3198) #3199

merged 8 commits into from
Mar 21, 2024

Conversation

nandorholozsnyak
Copy link
Contributor

  • Spring RestClient support

📜 Description

  • New classes are added to auto configure the neccessary interceptors for newly created RestClients
  • Tests are added to test the new functionality.

💡 Motivation and Context

  • The RestClient is similar to the RestTemplate but uses different ways of creation and because of that the older auto configurations did not put the required interceptors into these new RestClient instances

💚 How did you test it?

  • With the unit and IT tests

📝 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

nandorholozsnyak
nandorholozsnyak commented Feb 8, 2024
View reviewed changes
...boot-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentrySpanRestClientCustomizer.java Outdated

@Override
public void customize(final @NotNull RestClient.Builder restClientBuilder) {
restClientBuilder.requestInterceptors(clientHttpRequestInterceptors -> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a tricky one, I saw the same concept in the SentrySpanRestTemplateCustomizer but I'm not sure if the contains can catch that a SentrySpanClientHttpRequestInterceptor is already registered or not, as it is being instantiated in the constructor, so different instances can be registered easily. We might need a proper equals in the SentrySpanClientHttpRequestInterceptor

Copy link
Member

Choose a reason for hiding this comment

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

So far I'm not aware of any reported problems about this in SentrySpanRestTemplateCustomizer so I'd say we can do the same thing here and improve it if problems come up.

@adinauer
Copy link
Member

adinauer commented Feb 8, 2024

Thanks for the PR @nandorholozsnyak, we'll take a look but it might take a while as I'm feeling a bit under the weather.

@markushi markushi mentioned this pull request Feb 13, 2024
Closed
@adinauer adinauer self-assigned this Feb 19, 2024
Support Spring's new RestClient with auto configuration (getsentry#3198)
fbceb0a 
- Test when HTTP call fails is fixed with disabling retry mechanism in the Apache Http Client
@nandorholozsnyak
Copy link
Contributor Author

Thanks for the PR @nandorholozsnyak, we'll take a look but it might take a while as I'm feeling a bit under the weather.

Hello there,

Sorry for the late reply, but I could only solve the failing test today. Now I would like to ask for a review from you guys. I'll try to sync with master.

@adinauer
Copy link
Member

adinauer commented Mar 5, 2024

Hey @nandorholozsnyak thanks for the PR. We'll try to take a look soonish but we're in the middle of something right now, so might take a bit - sorry.

@adinauer adinauer mentioned this pull request Mar 21, 2024
Merged
3 tasks
adinauer
adinauer approved these changes Mar 21, 2024
View reviewed changes
Copy link
Member

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

Thanks again for the PR and also your patience. I've added a changelog, added RestClient to our sample and tweaked the reported trace origin.

I've opened a PR for docs: getsentry/sentry-docs#9531 which we should merge after this PR has been released.

...boot-jakarta/src/main/java/io/sentry/spring/boot/jakarta/SentrySpanRestClientCustomizer.java Outdated

@Override
public void customize(final @NotNull RestClient.Builder restClientBuilder) {
restClientBuilder.requestInterceptors(clientHttpRequestInterceptors -> {
Copy link
Member

Choose a reason for hiding this comment

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

So far I'm not aware of any reported problems about this in SentrySpanRestTemplateCustomizer so I'd say we can do the same thing here and improve it if problems come up.

@adinauer adinauer merged commit 955f1ff into getsentry:main Mar 21, 2024
23 checks passed
@nandorholozsnyak
Copy link
Contributor Author

Thanks again for the PR and also your patience. I've added a changelog, added RestClient to our sample and tweaked the reported trace origin.

I've opened a PR for docs: getsentry/sentry-docs#9531 which we should merge after this PR has been released.

Thank you so much.

@nandorholozsnyak nandorholozsnyak deleted the feat/spring-rest-client-auto-configuration-support branch March 21, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adinauer adinauer adinauer approved these changes

@romtsn romtsn Awaiting requested review from romtsn romtsn is a code owner

@stefanosiano stefanosiano Awaiting requested review from stefanosiano stefanosiano is a code owner

@markushi markushi Awaiting requested review from markushi markushi is a code owner

Assignees

@adinauer adinauer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nandorholozsnyak @adinauer