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

fix(otel): Don't require entrypoint config to initialize OTel #2579

Conversation

hairyhenderson
Copy link
Contributor

Description

Changes behaviour to always call otelService.Init, to avoid requiring setting the openTelemetryOtlpEndpoint config element.

If the OTel SDK is not explicitly disabled and no OTel endpoint is listening, the OTel SDK will complain in the logs. Since I don't want to unnecessarily spam logs, I'm adding support for the OTEL_LOG_LEVEL variable, but defaulting to debug rather than the spec's info for somewhat more sane logs.

Notes: I've not done it here, but I think deprecating openTelemetryOtlpEndpoint in the future is probably a good idea, in preference for otel.exporter.otlp.endpoint. I've also noticed that the otel config section is undocumented - I may have a look at that in a separate PR.

Closes issue(s)

Fixes #2578

Checklist

  • I have tested this code
  • I have added unit test to cover this code
  • I have updated the documentation (README.md and /website/docs)
  • I have followed the contributing guide

Copy link

netlify bot commented Oct 26, 2024

Deploy Preview for go-feature-flag-doc-preview canceled.

Name Link
🔨 Latest commit 6f03a74
🔍 Latest deploy log https://app.netlify.com/sites/go-feature-flag-doc-preview/deploys/6728d676a057890008c3cd86

Copy link

codecov bot commented Oct 26, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.09%. Comparing base (3bfac24) to head (6f03a74).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
cmd/relayproxy/api/server.go 40.00% 2 Missing and 1 partial ⚠️
cmd/relayproxy/api/opentelemetry/otel.go 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2579      +/-   ##
==========================================
- Coverage   85.11%   85.09%   -0.02%     
==========================================
  Files         101      101              
  Lines        4582     4583       +1     
==========================================
  Hits         3900     3900              
  Misses        544      544              
- Partials      138      139       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

@hairyhenderson I have tested this new version and my main concern is that we are still trying to call the OTLP when nothing is configured.

We have this log for every query:

{"level":"debug","ts":1730195483.37956,"caller":"opentelemetry/otel.go:93","msg":"OTel error","error":"traces export: Post \"https://localhost:4318/v1/traces\": dial tcp [::1]:4318: connect: connection refused"}

Maybe we should set the noop.NewTracerProvider() when we have no OTLP endpoint configured.

@hairyhenderson
Copy link
Contributor Author

We have this log for every query:

yeah, that's why I set it to log at debug level...

Maybe we should set the noop.NewTracerProvider() when we have no OTLP endpoint configured.

Yeah, the issue is that if there's no exporter, defaults will be used (OTLP over http/protobuf on https://localhost:4318). I had thought that perhaps some users would want this.

Realistically though, I think I'm the only user of GOFF that needs the OTel support so far, so requiring the endpoint to be set is probably fine.

I'll update this to check openTelemetryOtlpEndpoint or otel.exporter.otlp.endpoint for now.

@hairyhenderson hairyhenderson force-pushed the otel-dont-require-entrypoint-config-2578 branch from 1ac2243 to 4aad1e6 Compare November 1, 2024 21:45
@hairyhenderson hairyhenderson force-pushed the otel-dont-require-entrypoint-config-2578 branch from 4aad1e6 to e6db682 Compare November 1, 2024 21:46
@hairyhenderson
Copy link
Contributor Author

@thomaspoignant I've simplified things - PTAL 🙂

@thomaspoignant thomaspoignant force-pushed the otel-dont-require-entrypoint-config-2578 branch from a2ba621 to a786d0a Compare November 4, 2024 14:12
Copy link

sonarcloud bot commented Nov 4, 2024

Copy link
Owner

@thomaspoignant thomaspoignant left a comment

Choose a reason for hiding this comment

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

Looks really good now 👍

@thomaspoignant thomaspoignant merged commit 45b7084 into thomaspoignant:main Nov 4, 2024
20 of 22 checks passed
@hairyhenderson hairyhenderson deleted the otel-dont-require-entrypoint-config-2578 branch November 4, 2024 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(bug) Tracing only initializes when specifying endpoint through config
2 participants