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

Fix #261:Add support for traceparent headers in DefaultRestClient #262

Merged
merged 5 commits into from
Feb 1, 2024
Merged

Fix #261:Add support for traceparent headers in DefaultRestClient #262

merged 5 commits into from
Feb 1, 2024

Conversation

jandusil
Copy link
Contributor

No description provided.

@jandusil jandusil requested a review from banterCZ January 23, 2024 15:28
@jandusil jandusil self-assigned this Jan 23, 2024
@jandusil jandusil linked an issue Jan 23, 2024 that may be closed by this pull request
Add support for traceparent headers in DefaultRestClient #261
Closed
@jandusil jandusil requested a review from zcgandcomp January 23, 2024 23:49
banterCZ
banterCZ reviewed Jan 24, 2024
View reviewed changes
Copy link
Member

@banterCZ banterCZ left a comment

Choose a reason for hiding this comment

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

Not covered by tests.

rest-client-base/src/main/java/com/wultra/core/rest/client/base/DefaultRestClient.java Outdated Show resolved Hide resolved
rest-client-base/src/main/java/com/wultra/core/rest/client/base/DefaultRestClient.java Outdated Show resolved Hide resolved
rest-client-base/src/main/java/com/wultra/core/rest/client/base/DefaultRestClient.java Outdated Show resolved Hide resolved
rest-client-base/src/main/java/com/wultra/core/rest/client/base/RestClientConfiguration.java Outdated Show resolved Hide resolved
@jandusil
Copy link
Contributor Author

jandusil commented Jan 25, 2024 •
edited
Loading

Based on this blog I aimed to implement a way to propagate the traceId through Reactor context.
However, the blog was based on Sleuth solution, and it may be that the opentelemetry handles the non-blocking environment well without issues. Unfortunately, I have not found any evidence that would support nor contradict this claim. Therefore, I suggest adding this implementation, to be sure the context holds.

Can you please take a look at this solution? If it is accepted, I will add some tests as well.

romanstrobl
romanstrobl approved these changes Jan 28, 2024
View reviewed changes
Copy link
Member

@romanstrobl romanstrobl left a comment

Choose a reason for hiding this comment

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

The feature seems to be implemented based on the W3C specification.

I would suggest to make this feature optional (e.g. allow disabling adding the header using a property).

rest-client-base/src/main/java/com/wultra/core/rest/client/base/TraceparentFilterFunction.java Outdated Show resolved Hide resolved
@jandusil
Copy link
Contributor Author

jandusil commented Feb 1, 2024 •
edited
Loading

More info about the changes in the issue #261

TLDR:

  • Testing on multithreads OK✅ no need for Reactor context edits
  • No need to add custom properties, the tracing can be managed in app.properties

zcgandcomp
zcgandcomp approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@zcgandcomp zcgandcomp left a comment

Choose a reason for hiding this comment

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

Looks OK.

@jandusil jandusil merged commit dbb8a36 into develop Feb 1, 2024
4 checks passed
@jandusil jandusil deleted the issues/261-inject-traceparent-headers branch February 1, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romanstrobl romanstrobl romanstrobl approved these changes

@zcgandcomp zcgandcomp zcgandcomp approved these changes

@banterCZ banterCZ Awaiting requested review from banterCZ

Assignees

@jandusil jandusil

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for traceparent headers in DefaultRestClient
4 participants
@jandusil @banterCZ @zcgandcomp @romanstrobl