Conversation
The configuration macro is changed to a registry that includes types and default values next to the configuration names. This is used for compile time generation instead of using runtime checks for the configurations (environment variables). For safety, failing to parse configurations is now going to log a warning instead. This aligns with other platforms / tracers to not crash a customer in production where their staging configuration might be valid and their production one not. The macro must now be used for any new configuration added and a CI job verifies that all entries do exist. If a new env is added otherwise it should fail. The CI will also verify that the generated file is up to date and matches the remote registry. It would fail, if either is not the case. The behavior change is documented in the README.md.
dmehala
requested review from
zacharycmontoya
and removed request for
a team
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #284 +/- ##
==========================================
- Coverage 87.78% 87.76% -0.02%
==========================================
Files 84 84
Lines 5658 5658
==========================================
- Hits 4967 4966 -1
- Misses 691 692 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🎯 Code Coverage (details) 🔗 Commit SHA: daeb826 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
BenchmarksBenchmark execution time: 2026-02-27 14:55:49 Comparing candidate commit daeb826 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
There was a problem hiding this comment.
As discussed: I am fine with landing this right now without the additional improvements of the other PR by utilizing them in separate PRs.
The other PR has two benefits: we utilize compile time checks, the developers do not have to handle the type, the configurations are validated as early as possible, and the envs are checked more closely (the check was a bit brittle in the sense that changing the file could also easily break things). The README.md also contains less information here.
@dmehala and me discussed to have follow-up PRs where the configurations are improved a lot including the above improvements.
| if grep -R -n \ | ||
| --include='*.c' \ | ||
| --include='*.cc' \ | ||
| --include='*.cpp' \ | ||
| --include='*.cxx' \ | ||
| --include='*.h' \ | ||
| --include='*.hpp' \ | ||
| --exclude='environment.cpp' \ | ||
| 'std::getenv' include src; then |
There was a problem hiding this comment.
As discussed: this is not a sufficient test. Instead, we should use a linter where we are able to utilize an AST to prevent false positives.
Due to the low number of repo contributing people, this should be fine for now.
3 participants
Description and Motivation
Read #281