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 for Jaeger tracing integration in 3.x: shows incorrect port as of Helidon 3.0 and maybe should discuss removed config settings #5187

Closed
tjquinno opened this issue Oct 14, 2022 · 1 comment · Fixed by #6113
Assignees
Labels
3.x Issues for 3.x version branch docs tracing
Milestone

Comments

@tjquinno
Copy link
Member

tjquinno commented Oct 14, 2022

Environment Details

  • Helidon Version: 3.x, (4.x?)
  • Helidon SE or Helidon MP SE and MP
  • JDK version:
  • OS:
  • Docker version (if applicable):

Problem Description

  1. Helidon 3.x uses the Jaeger OpenTelemetry client to send information to the Jaeger server. That client uses port 14250 (correctly defaulted in the JaegerTracerBuilder class.

    But our doc for Jaeger tracing integration uses port 14240, not 14250 (or leaving it unset to use the correct default). See the config example snippets just below the config tables at https://helidon.io/docs/v3/#/se/tracing#_configuration_options_3 and https://helidon.io/docs/v3/#/mp/tracing#_configuration_options_3.

    We could simply remove the port setting. The default in the code works correctly and the config doc shows port as a config key so readers will know they can override the default if they need to.

  2. Several config keys that we supported in 2.x we no longer support because 3.x uses the OpenTelemetry tracing client rather than the native Jaeger one (as 2.x did). 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.

    The class-level JavaDoc for JaegerTracerBuilder has changed accordingly, but we mention nothing in the doc on our website. See the JaegerTracerBuilder source diffs to see which keys are no longer supported. Here is a copy of the config code that shows which keys were supported in 2.x but not in 3.x:

         config.get("token").asString().ifPresent(this::token);
         config.get("username").asString().ifPresent(this::username);
         config.get("password").asString().ifPresent(this::password);
         config.get("propagation").asList(String.class).ifPresent(propagations -> {
             propagations.stream()
                     .map(String::toUpperCase)
                     .map(Configuration.Propagation::valueOf)
                     .forEach(this::addPropagation);
    
         });
         config.get("log-spans").asBoolean().ifPresent(this::logSpans);
         config.get("max-queue-size").asInt().ifPresent(this::maxQueueSize);
         config.get("flush-interval-ms").asLong().ifPresent(this::flushIntervalMs);
    ...
         config.get("sampler-manager").asString().ifPresent(this::samplerManager);
    

Steps to reproduce

  • Enhance the MP QuickStart example by adding the following to microprofile-config.properties: (this came from a user for whom this set-up worked correctly in Helidon 2.x)

    tracing.service=s1-auth-svc
    tracing.protocol=http
    tracing.host=localhost
    tracing.port=14240
    tracing.sampler-type=const
    tracing.sampler-param=1
    tracing.components.security.enabled=false
    tracing.paths.1.path=/metrics
    tracing.paths.1.enabled=false
    tracing.paths.2.path=/health
    tracing.paths.2.enbled=false
    
  • Start Jaeger (make sure its log reports it is listening on 14250).

  • Build and run the modified MP QuickStart app.

  • curl http://localhost:8080/greet

  • The QuickStart output shows a failure to report the tracing data.

@tjquinno tjquinno added docs tracing 3.x Issues for 3.x version branch labels Oct 14, 2022
@tjquinno tjquinno changed the title Doc for Jaeger integration uses incorrect port as of Helidon 3.0 Doc for Jaeger tracing integration uses incorrect port as of Helidon 3.0 Oct 18, 2022
@tjquinno tjquinno changed the title Doc for Jaeger tracing integration uses incorrect port as of Helidon 3.0 Doc for Jaeger tracing integration: shows incorrect port as of Helidon 3.0 and maybe should discuss removed config settings Oct 18, 2022
@tjquinno tjquinno changed the title Doc for Jaeger tracing integration: shows incorrect port as of Helidon 3.0 and maybe should discuss removed config settings Doc for Jaeger tracing integration in 3.x: shows incorrect port as of Helidon 3.0 and maybe should discuss removed config settings Oct 18, 2022
@barchetta barchetta added this to the 3.1.1 milestone Jan 19, 2023
@barchetta barchetta modified the milestones: 3.1.1, 3.1.2 Feb 4, 2023
@ljamen
Copy link
Contributor

ljamen commented Feb 9, 2023

Fixed by #6113

@ljamen ljamen closed this as completed Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues for 3.x version branch docs tracing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants