refactor: Remote Configuration#130
Conversation
Refactor the Remote Configuration (RC) as a standalone module, enabling
its use independently of the tracer. Additionally, ensure that external
listeners can register and receive configuration updates when RC is
embedded in the tracer.
Changes:
- Move APM_TRACING updates handling to the configuration manager.
- Expose the `remote_configuration_listeners` field in datadog agent config
to allow registration of external listeners.
- Improve the robustness of config path parsing and ensure an error is reported
if parsing fails.
- Add `to_upper` utility function.
- Increase test coverage for RC and the new configuration manager
behaviour.
dmehala
requested review from
cataphract and
pablomartinezbernardo
and removed request for
a team
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #130 +/- ##
==========================================
+ Coverage 94.48% 94.52% +0.03%
==========================================
Files 71 73 +2
Lines 3720 3798 +78
==========================================
+ Hits 3515 3590 +75
- Misses 205 208 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| switch (product) { DD_LIST_REMOTE_CONFIG_PRODUCTS } | ||
| #undef X | ||
|
|
||
| std::abort(); |
There was a problem hiding this comment.
I know this is quite unlikely but abort is extreme as it can't be recovered from, perhaps an exception would be better?
There was a problem hiding this comment.
If it does happen I don't think an exception can help much. The program is in a state where the continuity and integrity of it is questionable.
There was a problem hiding this comment.
Certainly that applies to the remote config client, but otherwise you can't make the assertion beyond that. It makes more sense to capture said exception and disable the RC client than to crash the whole thing irrevocably.
There was a problem hiding this comment.
I don't agree but I can understand how frightening std::abort can be. I addressed your concern in d547482
| } | ||
|
|
||
| // Keep track of config path received to know which ones to revert. | ||
| std::unordered_set<std::string> visited_config; |
There was a problem hiding this comment.
I'm not familiar with this json library, but if the string views you're using down below persist after the loop it might be worth changing this as well to use views.
There was a problem hiding this comment.
Good catch. Thank you.
There was a problem hiding this comment.
Addressed in a32272c
EDIT: Ah! I remember abseil's string view has no hash implementation. That's why I sticked with a string.
| using namespace rc::capability; | ||
| return APM_TRACING_SAMPLE_RATE | APM_TRACING_TAGS | APM_TRACING_ENABLED | | ||
| APM_TRACING_SAMPLE_RULES; | ||
| }; |
There was a problem hiding this comment.
| }; | |
| } |
Stray ; is causing the benchmarks to fail to build.
BenchmarksBenchmark execution time: 2024-07-02 12:16:02 Comparing candidate commit 2572cab in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics. |
dmehala
force-pushed
the
dmehala/remote-config-module
branch
from
1901645 to
2572cab
Compare
Resolve a regression introduced in #130 where the `configuration_change` payload didn't adhere to the telemetry schema.
Resolve a regression introduced in #130 where the `configuration_change` payload didn't adhere to the telemetry schema.
5 participants
Description
Refactor the Remote Configuration (RC) as a standalone module, enabling its use independently of the tracer. Additionally, ensure that external listeners can register and receive configuration updates when RC is embedded in the tracer.
Motivation
ASM needs a way to receives RC updates for NGINX integration.
Changes:
remote_configuration_listenersfield in datadog agent config to allow registration of external listeners.to_upperutility function.