-
Notifications
You must be signed in to change notification settings - Fork 0
feat(remote_config): send RC updates via telemetry (sampling rules) #89
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
Conversation
|
7c53428 to
a55a3c4
Compare
a55a3c4 to
968c92b
Compare
| /// - `config_id`: Optional identifier for remote configuration scenarios. When provided, this | ||
| /// ID is included in the returned `Configuration` to track which remote configuration is | ||
| /// responsible for the current value. | ||
| fn get_configuration(&self, config_id: Option<String>) -> Configuration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having to provide a config_id here which is directly used to fill one of the field of the returned Configuration value feels weird
I would expect ConfigItems to carry their config_id with them, and use it to construct the Configuration value when calling get_configuration.
This will be useful when we enable stable config for instance, where the configurations items we read can have their own config id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, as I don't know well all the different cases for configuration I tried not to store config_id (and not to mess around with ArcSwap) but I have changed it.
For now, only ConfigItemWithOverride can update its config_id. idk if it would be necessary also for ConfigItem...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
dd-trace/src/telemetry.rs
Outdated
| let Some(telemetry) = telemetry_cell.get() else { | ||
| return; | ||
| }; | ||
| let Ok(mut telemetry) = telemetry.lock() else { | ||
| return; | ||
| }; | ||
| if !telemetry.enabled { | ||
| return; | ||
| } | ||
| let Some(handle) = telemetry.handle.as_mut() else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: you can replace this with with_telemetry_handle now that the other PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
What does this PR do?
Get the
idof the last RC update and use it to notify the configuration change via telemetry:ApmTracingConfigstruct modified to include theidfield received in the RC responsetelemetry::notify_configuration_updatemethod to notify the configuration change from theupdate_sampling_rules_from_remoteandclear_remote_sampling_rulescallbacks using the same config_id received previouslyMotivation
Make pass some
test_dynamic_configuration.py::TestDynamicConfigSamplingRulestestsAdditional Notes
libdatadog's
TelemetryWorkerHandlemanages internally the configuration property changes. It stores the initial configuration values and it is able to know when a value has changed and to emit theapp-client-configuration-changetelemetry event.