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 for jaeger remote sampler #36476

Merged
merged 1 commit into from
Oct 27, 2023
Merged

Support for jaeger remote sampler #36476

merged 1 commit into from
Oct 27, 2023

Conversation

hiteshkhatri97
Copy link
Contributor

Fixes #36335

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 13, 2023

/cc @brunobat (opentelemetry), @radcortez (opentelemetry)

@hiteshkhatri97 hiteshkhatri97 force-pushed the add-new-sampler-jaeger-remote branch 2 times, most recently from 60b1340 to 7d0ad03 Compare October 13, 2023 16:09
@hiteshkhatri97
Copy link
Contributor Author

hiteshkhatri97 commented Oct 13, 2023 •
edited
Loading

This is part of the ongoing hacktoberfest. Please add the label hacktoberfest-accepted before merging

@hiteshkhatri97
Copy link
Contributor Author

@brunobat @radcortez please review

brunobat
brunobat requested changes Oct 14, 2023
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@hiteshkhatri97 The opentelemetry-sdk-extension-jaeger-remote-sampler must not be part of the main extension.
It's fine to change the config values to string but you need to handle the consequences o the current default sampler.
On the OpenTelemetry Integration test, you can include opentelemetry-sdk-extension-jaeger-remote-sampler and create a test in there to see if it works.

@hiteshkhatri97
Copy link
Contributor Author

@brunobat are you saying opentelemetry-sdk-extension-jaeger-remote-sampler is not required in this case or we shouldn't add support for this? Sorry, I'm new and just looking for some clarification.

@hiteshkhatri97
Copy link
Contributor Author

hiteshkhatri97 commented Oct 14, 2023 •
edited
Loading

If I understand correctly, if someone wants to configure the jaeger_remote sampler, they must include its dependency in their project, right?

@quarkus-bot

This comment has been minimized.

Sign in to view
@hiteshkhatri97
Copy link
Contributor Author

Also, just wondering if integration tests are required then they should be part of this PR or separate PR?

@brunobat
Copy link
Contributor

Tests must be part of the PR.
The main point of the PR, in my opinion, is to allow the arg to hold a String value and deal with potential side effects on the current code.
After this change, other samplers can take benefit of the String 'arg'. To prove this is the case, you can create an integration test that will use opentelemetry-sdk-extension-jaeger-remote-sampler.

@hiteshkhatri97 hiteshkhatri97 force-pushed the add-new-sampler-jaeger-remote branch from 4d0f0c5 to 4aa02df Compare October 21, 2023 12:57
@hiteshkhatri97 hiteshkhatri97 marked this pull request as draft October 21, 2023 13:04
@hiteshkhatri97
Copy link
Contributor Author

@brunobat This is ready for early review.

@brunobat
Copy link
Contributor

brunobat commented Oct 24, 2023 •
edited
Loading

hiteshkhatri97 will checkout the project and see what happens with the test.
In the meantime, can you please squash your commits?

@hiteshkhatri97 hiteshkhatri97 force-pushed the add-new-sampler-jaeger-remote branch from f5e5ae3 to 1fcae10 Compare October 24, 2023 07:35
@hiteshkhatri97
Copy link
Contributor Author

Thanks @brunobat. Now the tests look fine to me. I had kept initialSamplingRate very low. I have changed it now.

@brunobat
Copy link
Contributor

Excelent, will approve the CI run and see how it goes. BTW, we require all commits to be squashed into a single commit per PR.

@hiteshkhatri97 hiteshkhatri97 force-pushed the add-new-sampler-jaeger-remote branch from 1fcae10 to ad8c33e Compare October 24, 2023 08:14
@hiteshkhatri97
Copy link
Contributor Author

Oh, I thought commits could be squashed while merging the PR. Nvm I will do it

@hiteshkhatri97 hiteshkhatri97 force-pushed the add-new-sampler-jaeger-remote branch 3 times, most recently from e2f8e1a to 37feac1 Compare October 24, 2023 19:41
@hiteshkhatri97 hiteshkhatri97 marked this pull request as ready for review October 24, 2023 19:43
@hiteshkhatri97
Copy link
Contributor Author

@brunobat commits are squashed. Let me know what you think

@hiteshkhatri97
Copy link
Contributor Author

@brunobat can we possibly try to close this today if everything looks good?

brunobat
brunobat requested changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

Sorry @hiteshkhatri97, took a deep look at the test and I think it needs to be improved.

...pentelemetry-jaeger-remote/src/test/java/io/quarkus/it/opentracing/OTelJaegerRemoteTest.java Outdated Show resolved Hide resolved
...pentelemetry-jaeger-remote/src/test/java/io/quarkus/it/opentracing/OTelJaegerRemoteTest.java Outdated Show resolved Hide resolved
...pentelemetry-jaeger-remote/src/test/java/io/quarkus/it/opentracing/OTelJaegerRemoteTest.java Outdated Show resolved Hide resolved
...pentelemetry-jaeger-remote/src/test/java/io/quarkus/it/opentracing/OTelJaegerRemoteTest.java Outdated Show resolved Hide resolved
@quarkus-bot

This comment has been minimized.

Sign in to view
@hiteshkhatri97 hiteshkhatri97 force-pushed the add-new-sampler-jaeger-remote branch from 743c50d to 9dd95fe Compare October 26, 2023 06:13
brunobat
brunobat requested changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

Getting closer.

...try-jaeger-remote/src/test/java/io/quarkus/it/opentelemetry/jaeger/OTelJaegerRemoteTest.java Outdated Show resolved Hide resolved
...try-jaeger-remote/src/test/java/io/quarkus/it/opentelemetry/jaeger/OTelJaegerRemoteTest.java Outdated Show resolved Hide resolved
@hiteshkhatri97 hiteshkhatri97 force-pushed the add-new-sampler-jaeger-remote branch from 4f3bed3 to bd3ffb4 Compare October 26, 2023 07:42
@quarkus-bot

This comment has been minimized.

Sign in to view
@hiteshkhatri97
Copy link
Contributor Author

@brunobat Sorry I'm bugging you again but if you can take a look. Thanks

brunobat
brunobat requested changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

The test looks good.

...ntime/src/main/java/io/quarkus/opentelemetry/runtime/config/runtime/TracesRuntimeConfig.java Show resolved Hide resolved
@hiteshkhatri97 hiteshkhatri97 force-pushed the add-new-sampler-jaeger-remote branch from 27ffa7a to 72b9d56 Compare October 26, 2023 10:55
brunobat
brunobat approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @hiteshkhatri97 for your effort!

@hiteshkhatri97 hiteshkhatri97 force-pushed the add-new-sampler-jaeger-remote branch 2 times, most recently from 8dfdd71 to 9c5d556 Compare October 26, 2023 11:02
@hiteshkhatri97 hiteshkhatri97 force-pushed the add-new-sampler-jaeger-remote branch from 9c5d556 to c57586f Compare October 26, 2023 11:03
@hiteshkhatri97
Copy link
Contributor Author

Thanks a lot, @brunobat for your review, help, and support. Will merge it once CI run completes if I have access to merge

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 26, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@hiteshkhatri97
Copy link
Contributor Author

@brunobat can you please merge it? Looks like I don't have access

@brunobat
Copy link
Contributor

Usually that's up to @gsmet or @geoand

geoand
geoand reviewed Oct 26, 2023
View reviewed changes
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

I added a comment

...metry-jaeger-remote/src/test/java/io/quarkus/it/opentelemetry/jaeger/OTelJaegerRemoteIT.java

import io.quarkus.test.junit.QuarkusIntegrationTest;

@QuarkusIntegrationTest
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this is meant to test things in native mode?

If so, then we'll need to configure the proper native JSON config file in the .github directory

Copy link
Contributor

@brunobat brunobat Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is quite specific. Probably no need. There are other IT related projects for OTel that are not running on Native.
There are quite a few OTel IT projects now.

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@hiteshkhatri97
Copy link
Contributor Author

@geoand / @gsmet Can you please merge it? Let me know if there are any concerns.

@geoand
Copy link
Contributor

geoand commented Oct 27, 2023

@brunobat will take care of that

@brunobat brunobat merged commit bbdc2ae into quarkusio:main Oct 27, 2023
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand left review comments

@brunobat brunobat brunobat approved these changes

Assignees

@hiteshkhatri97 hiteshkhatri97

Labels
area/tracing kind/bugfix
Projects
Quarkus Roadmap/Planning
Status: Done
Milestone
3.6.0.CR1
Development

Successfully merging this pull request may close these issues.

quarkus.otel.traces.sampler.arg doesn't support String value (e.g. cannot configure "jaeger_remote" sampler)
3 participants
@hiteshkhatri97 @brunobat @geoand