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

Samplers use Locale dependent formatting for description #4880

Closed
lmonkiewicz opened this issue Oct 23, 2022 · 4 comments · Fixed by #4887
Closed

Samplers use Locale dependent formatting for description #4880

lmonkiewicz opened this issue Oct 23, 2022 · 4 comments · Fixed by #4887
Assignees
Labels
Bug Something isn't working

Comments

@lmonkiewicz
Copy link
Contributor

Describe the bug
RateLimiingSampler and TraceIdRationBasedSampler use String.format("RateLimitingSampler{%.2f}", maxTracesPerSecond) for building a description. Formatting of numbers in such way may result in 999.00 for en_US locale and 999,00 for pl_PL.

I propose to enforce locale for those two samplers, using String.format(Locale.ENGLISH, "RateLimitingSampler{%.2f}", maxTracesPerSecond).

If that solution is acceptable, I can take care of it.

Steps to reproduce
Change locale on your JVM to pl_PL and run JaegerRemoteSamplerTest test suite. Most tests will fail on checking of description formats.

What did you expect to see?
Behavior independent of locale.

What did you see instead?

Expecting actual:
  "JaegerRemoteSampler{ParentBased{root:RateLimitingSampler{999,00},remoteParentSampled:AlwaysOnSampler,remoteParentNotSampled:AlwaysOffSampler,localParentSampled:AlwaysOnSampler,localParentNotSampled:AlwaysOffSampler}}"
to contain:
  "RateLimitingSampler{999.00}" 

What version and what artifacts are you using?
Artifacts: opentelemetry-java, jaeger-remote-sampler
Version: fdfacb0bdea770f354e27305b5aab77a9d965a4f

Environment
Compiler: OpenJDK 18.0.1
OS: MacOS

@lmonkiewicz lmonkiewicz added the Bug Something isn't working label Oct 23, 2022
@jack-berg
Copy link
Member

Not saying I disagree with a standard locale, but curious how this impacted you in practice?

@lmonkiewicz
Copy link
Contributor Author

Not saying I disagree with a standard locale, but curious how this impacted you in practice?

Not a real usage case, but when I was working on another issue, Jaeger tests were failing all the time. Took me a while before I noticed why.

@jack-berg
Copy link
Member

Ah I missed that JaegerRemoteSamplerTest asserts on the locale specific string! Seems reasonable to make the formatting consistent. How about using something like new DecimalFormat("#.#######").format(..) rather than tying the code to a specific locale?

@lmonkiewicz
Copy link
Contributor Author

Ah I missed that JaegerRemoteSamplerTest asserts on the locale specific string! Seems reasonable to make the formatting consistent. How about using something like new DecimalFormat("#.#######").format(..) rather than tying the code to a specific locale?

Fine by me, you can assign it to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants