Add Locale.ROOT to ConfigStrings Environment Variable Normalization#11979
Conversation
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
There was a problem hiding this comment.
More details
The PR correctly fixes a locale-sensitive bug where Turkish systems would corrupt config telemetry by converting 'i' → 'İ' (dotted I) instead of 'I'. Adding Locale.ROOT to toUpperCase() and toLowerCase() ensures all environment variable name conversions use ASCII-invariant transformations. The deprecated systemPropertyNameToEnvironmentVariableName() is safely removed and replaced with the fixed toEnvVar() at its only call site. The new test forces Turkish locale and verifies the fix prevents the corruption.
📊 Validated against 6 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit f326662 · What is Autotest? · Any feedback? Reach out in #autotest
bm1549
left a comment
There was a problem hiding this comment.
LGTM since it solves the issue at hand, but I do wonder how many of the remaining toUpperCase / toLowerCase instances may be causing similar problems we just haven't found yet
https://github.com/search?q=repo%3ADataDog%2Fdd-trace-java+Case%28%29&type=code
|
🎯 Code Coverage (details) 🔗 Commit SHA: d2e62e8 | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
| @BeforeEach | ||
| void forceTurkishLocale() { |
There was a problem hiding this comment.
❔ question: Not sure it makes sense to force the Turkish locale for all tests (the current one and all comings next)
What about limiting it to the related test case instead?
There was a problem hiding this comment.
Good Catch, I'll just add it to the test case itself
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 92d254c: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
e41dc82
into
master
What Does This Do
Previously, Turkish Locale would uppercase
i-> a dottedI. This led to issues w/ our telemetry monitors after hitting the backend, sparking false alarms that were were not properly capturing config telemetry.This PR also replaces a duplicate function
systemPropertyNameToEnvironmentVariableNametotoEnvVar.Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]