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

Doc updates for Jaeger tracing port value #6060

Closed
wants to merge 9 commits into from

Conversation

ljamen
Copy link
Contributor

@ljamen ljamen commented Feb 2, 2023

Doc for issue #5187 and #6094

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 2, 2023
@ljamen ljamen requested a review from tjquinno February 2, 2023 19:18
@ljamen
Copy link
Contributor Author

ljamen commented Feb 2, 2023

Hi @tjquinno - There are a couple of ways to address this doc bug. I believe you were asking the team how to update. Did you get your answer?

Should the doc contain a brief note at least listing these and/or explaining workarounds if any? For example, propagation is no longer supported, but propagation: b3 is no longer needed because the Helidon's JaegerTracerBuilder class always includes the OpenTelemetry B3Propagator.

@ljamen ljamen added this to the 3.1.2 milestone Feb 2, 2023
@tjquinno
Copy link
Member

tjquinno commented Feb 2, 2023

TBH I am not sure what workarounds, if any, are available for the settings that Jaeger no longer supports. Our config properties, then and now, simply mirror whatever Helidon's code can set on Jaeger's span exporter builder. Jaeger changed and so the settings we can expose changed as well.

Given that, it's probably clearer to not list the specific settings we no longer support (given that, at least at the moment, I don't know what, if any, workaround there might be). Maybe, just before the include of the generated config .adoc file, we could state something like this:

Because Jaeger changed its client implementation some Jaeger settings exposed by earlier releases of Helidon are no longer available. Please note the currently-supported settings in the table below.

or something to that effect.

docs/includes/tracing/tracer-jaeger.adoc Show resolved Hide resolved
docs/se/tracing.adoc Outdated Show resolved Hide resolved
@al3xandru
Copy link

This commit doesn't include a fix for https://helidon.io/docs/v3/#/se/guides/tracing (the snippet using private void getDefaultMessageHandler refers to and uese the io.opentracing instead of io.helidon.tracing API)

@ljamen
Copy link
Contributor Author

ljamen commented Feb 7, 2023

Hi @al3xandru I'm working with Tim on that. This is still in draft mode until I've completed all of the updates. They will be included in this PR.

@ljamen ljamen marked this pull request as ready for review February 7, 2023 21:02
@ljamen
Copy link
Contributor Author

ljamen commented Feb 7, 2023

Closing this PR and will include files in a new PR that does not include the generated tracing doc that was accidentally updated.

@ljamen ljamen closed this Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants