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

feat(tracing): Misc. OTel tracing improvements #2485

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hairyhenderson
Copy link
Contributor

@hairyhenderson hairyhenderson commented Oct 9, 2024

Description

I ran into a few papercuts while setting up tracing with the relay proxy after getting it up and running with #2482, most notably I use OTLP over gRPC rather than the default http/protobuf protocol.

So, this PR adds a few improvements:

  • ability to disable with OTEL_SDK_DISABLED=true
  • set endpoint with OTEL_EXPORTER_OTLP_ENDPOINT rather than config flag (but config flag is still supported)
  • set protocol with OTEL_EXPORTER_OTLP_PROTOCOL (to grpc, for example)
  • override service name with OTEL_SERVICE_NAME
  • adds more resource attributes by default, including supporting setting from the OTEL_RESOURCE_ATTRIBUTES env var
  • adds support for configuring sampling strategy, including with the Jaeger remote sampling protocol
  • hooks up the zap logger to OTel's error handling so log messages are formatted nicely, rather than just being logged with the stdlib log.Print

This also fixes a bug where apiServer.Stop() was not properly called. I've also modified the signature to receive a context so we can give the shutdown a 5s timeout.

Closes issue(s)

None directly, but related to #2479

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 9, 2024

Deploy Preview for go-feature-flag-doc-preview ready!

Name Link
🔨 Latest commit 0909368
🔍 Latest deploy log https://app.netlify.com/sites/go-feature-flag-doc-preview/deploys/6716ca738c158e0008b6519b
😎 Deploy Preview https://deploy-preview-2485--go-feature-flag-doc-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 81.55340% with 19 lines in your changes missing coverage. Please review.

Project coverage is 85.14%. Comparing base (c1c2389) to head (0909368).

Files with missing lines Patch % Lines
cmd/relayproxy/api/opentelemetry/otel.go 86.31% 10 Missing and 3 partials ⚠️
cmd/relayproxy/main.go 0.00% 5 Missing ⚠️
cmd/relayproxy/api/server.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2485      +/-   ##
==========================================
+ Coverage   84.81%   85.14%   +0.33%     
==========================================
  Files         100      100              
  Lines        4346     4424      +78     
==========================================
+ Hits         3686     3767      +81     
+ Misses        535      529       -6     
- Partials      125      128       +3     

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

@hairyhenderson hairyhenderson marked this pull request as ready for review October 9, 2024 18:01
@hairyhenderson hairyhenderson force-pushed the feat-otlp-support-grpc branch 3 times, most recently from ee84488 to bd6c73c Compare October 9, 2024 20:28
@thomaspoignant
Copy link
Owner

Hey @hairyhenderson thanks a lot for this PR.
I still need to look at it more in details, but I am not a big fan of using os.GetEnv outside of the configuration object.

I would prefer to have all those options in the Config object rather than dealing with env variable directly.

We will still be able to set them via environment variable because we are using knadh/koanf that will allow us to set the values through file and/or environment variable to.

@thomaspoignant
Copy link
Owner

I've played around with koanf and we could have something like this in the config:

type Config struct {
// ...
	OtelConfig OpenTelemetryConfiguration `mapstructure:"otel" koanf:"otel"`
}


type OpenTelemetryConfiguration struct {
	SDK struct {
		Disabled bool `mapstructure:"disabled" koanf:"disabled"`
	} `mapstructure:"sdk" koanf:"sdk"`
	Exporter struct {
		Otlp struct {
			Endpoint string `mapstructure:"endpoint" koanf:"endpoint"`
			Protocol string `mapstructure:"protocol" koanf:"protocol"`
		} `mapstructure:"otlp" koanf:"otlp"`
	} `mapstructure:"exporter" koanf:"exporter"`
	Service struct {
		Name string `mapstructure:"name" koanf:"name"`
	} `mapstructure:"service" koanf:"service"`
	Traces struct {
		Sampler string `mapstructure:"sampler" koanf:"sampler"`
	} `mapstructure:"sampler" koanf:"sampler"`
	Resource struct {
		Attributes map[string]string `mapstructure:"attributes" koanf:"attributes"`
	} `mapstructure:"resource" koanf:"resource"`
}

This will allow to continue to set all the configuration through environment variable like you do, but at the same time be able to configure directly inside the configuration file also.

What do you think about this idea @hairyhenderson ?
I am happy to help with that if you want.

@hairyhenderson
Copy link
Contributor Author

What do you think about this idea @hairyhenderson ?

I'm OK either way... It's somewhat more common (in my experience) to see OTel configured through environment variables, but I understand your desire for both modes.

I'm not familiar with koanf so integrating this will take me a bit of time, and I don't have a lot of spare time to spend on this. I'll try to find some time over the next little while to get this updated...

@thomaspoignant
Copy link
Owner

@hairyhenderson I can do it if you prefer. I am happy to update your PR with the changes if you don't have time.

Copy link

sonarcloud bot commented Oct 21, 2024

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.

2 participants