Skip to content

Fix: error logged on empty OTEL_TRACES_SAMPLER_ARG - #6511

Merged
dmathieu merged 2 commits into
open-telemetry:mainfrom
matthewhughes934:fix-empty-env-var-error
Jan 6, 2025
Merged

Fix: error logged on empty OTEL_TRACES_SAMPLER_ARG#6511
dmathieu merged 2 commits into
open-telemetry:mainfrom
matthewhughes934:fix-empty-env-var-error

Conversation

@matthewhughes934

Copy link
Copy Markdown
Contributor

We were noticing errors like:

time=2024-12-18T15:00:02.080Z level=ERROR
source=/go/pkg/mod/go.opentelemetry.io/contrib/samplers/jaegerremote@v0.27.0/sampler_remote_options.go:112 msg="env variable parsing failure" err="argument is not of type '='"

In applications using the Jaeger sampler but had not set the OTEL_TRACES_SAMPLER_ARG environment variable. The issue was:

  • os.Getenv would return "" when this var was not set, and
  • strings.Split("", ",") would return []string{""}

since "" does not contain = an error is emitted.

@matthewhughes934
matthewhughes934 requested review from a team and yurishkuro as code owners December 18, 2024 17:36
Comment thread samplers/jaegerremote/sampler_remote_options.go Outdated

@MrAlias MrAlias left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall, this looks good. Thank you for adding tests to cover your issue. The last remaining thing is a changelog entry for the fix your adding needs to be included.

@codecov

codecov Bot commented Dec 19, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.5%. Comparing base (dee94b1) to head (4eb890c).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #6511     +/-   ##
=======================================
- Coverage   68.5%   68.5%   -0.1%     
=======================================
  Files        200     200             
  Lines      16768   16772      +4     
=======================================
+ Hits       11493   11494      +1     
- Misses      4931    4933      +2     
- Partials     344     345      +1     
Files with missing lines Coverage Δ
samplers/jaegerremote/sampler_remote_options.go 97.2% <100.0%> (-2.8%) ⬇️

... and 1 file with indirect coverage changes

@matthewhughes934

Copy link
Copy Markdown
Contributor Author

Overall, this looks good. Thank you for adding tests to cover your issue. The last remaining thing is a changelog entry for the fix your adding needs to be included.

Thanks! Changelog entry added with 4d72fb9

We were noticing errors like:

> time=2024-12-18T15:00:02.080Z level=ERROR
source=/go/pkg/mod/go.opentelemetry.io/contrib/samplers/jaegerremote@v0.27.0/sampler_remote_options.go:112
msg="env variable parsing failure" err="argument  is not of type
'<key>=<value>'"

In applications using the Jaeger sampler but had not set the
`OTEL_TRACES_SAMPLER_ARG` environment variable. The issue was:

* `os.Getenv` would return `""` when this var was not set, and
* `strings.Split("", ",")` would return `[]string{""}`

since `""` does not contain `=` an error is emitted.
@dmathieu
dmathieu merged commit 96acbc0 into open-telemetry:main Jan 6, 2025
@MrAlias MrAlias added this to the v1.34.0 milestone Jan 17, 2025
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.

3 participants