Skip to content

Commit

Permalink
fix(remoteconfig): fix remoteconfig when flare data in payload (DataD…
Browse files Browse the repository at this point in the history
…og#9196)

Remote config was broken entirely due to the addition of tracer flare
data in the config sent from the agent. Example incoming RC config:
```
data = {'metadata': [{'id': 'configuration_order', 'product_name': 'AGENT_CONFIG', 'sha256_hash': 'ddfc2c7b5ee1710aa915edfccd8a0d452784d946cebae0554485b5c0539a9e2c', 'length': 198, 'tuf_version': 2, 'apply_state': 2, 'apply_error': None}, {'id': 'f6c80fdcc00b702c54ff6ae5ff2ac7f16d9afef109bdf53ee990376455301ab2', 'product_name': 'APM_TRACING', 'sha256_hash': '098cb5a0d27fce648cdd4c6e686038282b64ffee7f42b7238a78552c91948d11', 'length': 616, 'tuf_version': 3, 'apply_state': 2, 'apply_error': None}], 'config': [{'internal_order': ['flare-log-level.trace', 'flare-log-level.debug', 'flare-log-level.info', 'flare-log-level.warn', 'flare-log-level.error', 'flare-log-level.critical', 'flare-log-level.off'], 'order': []}, {'id': 'f6c80fdcc00b702c54ff6ae5ff2ac7f16d9afef109bdf53ee990376455301ab2', 'revision': 1715109076236, 'schema_version': 'v1.0.0', 'action': 'enable', 'lib_config': {'library_language': 'all', 'library_version': 'latest', 'service_name': 'zachs-python-app', 'env': 'zachariah', 'tracing_enabled': True, 'dynamic_sampling_enabled': False, 'tracing_tags': ['rc:works'], 'tracing_sampling_rules': [{'service': 'zachs-python-app', 'provenance': 'customer', 'resource': 'GET /', 'sample_rate': 0.01}, {'service': 'zachs-python-app', 'provenance': 'customer', 'resource': '', 'sample_rate': 1}]}, 'service_target': {'service': 'zachs-python-app', 'env': 'zachariah'}}], 'shared_data_counter': 1}
```

The python tracer’s implementation of pulling RC rules was brittle and
relied upon data["config"][0] always having the lib_config dict.
However, with tracer flares it seems the agent sometimes sends the
payload with the flare info in that 0 position in the list, so instead
we need to sometimes grab data["config"][1] .


## Checklist

- [ ] Change(s) are motivated and described in the PR description
- [ ] Testing strategy is described if automated tests are not included
in the PR
- [ ] Risks are described (performance impact, potential for breakage,
maintainability)
- [ ] Change is maintainable (easy to change, telemetry, documentation)
- [ ] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [ ] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [ ] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [ ] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.

## Reviewer Checklist

- [x] Title is accurate
- [x] All changes are related to the pull request's stated goal
- [x] Description motivates each change
- [x] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [x] Testing strategy adequately addresses listed risks
- [x] Change is maintainable (easy to change, telemetry, documentation)
- [x] Release note makes sense to a user of the library
- [x] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [ ] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
  • Loading branch information
ZStriker19 authored May 8, 2024
1 parent ef8a71e commit 69f91ee
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 4 deletions.
10 changes: 8 additions & 2 deletions ddtrace/settings/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -743,8 +743,15 @@ def _handle_remoteconfig(self, data, test_tracer=None):
log.warning("unexpected number of RC payloads %r", data)
return

# Check if 'lib_config' is a key in the dictionary since other items can be sent in the payload
config = None
for config_item in data["config"]:
if isinstance(config_item, Dict):
if "lib_config" in config_item:
config = config_item
break

# If no data is submitted then the RC config has been deleted. Revert the settings.
config = data["config"][0]
base_rc_config = {n: None for n in self._config}

if config and "lib_config" in config:
Expand Down Expand Up @@ -777,7 +784,6 @@ def _handle_remoteconfig(self, data, test_tracer=None):
if tags:
tags = self._format_tags(lib_config["tracing_header_tags"])
base_rc_config["trace_http_header_tags"] = tags

self._set_config_items([(k, v, "remote_config") for k, v in base_rc_config.items()])
# called unconditionally to handle the case where header tags have been unset
self._handle_remoteconfig_header_tags(base_rc_config)
Expand Down
8 changes: 8 additions & 0 deletions releasenotes/notes/rc_payload_fix-a03a98dcad934658.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
fixes:
- |
RemoteConfig: This fix resolves an issue where remote config did not work for the tracer when using an agent
that would add a flare item to the remote config payload. With this fix, the tracer will now correctly pull out
the lib_config we need from the payload in order to implement remote config changes properly.
18 changes: 16 additions & 2 deletions tests/internal/test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,25 @@ def _base_rc_config(cfg):
return {
"metadata": [],
"config": [
# this flare data can often come in and we want to make sure we're pulling the
# actual lib_config data out correctly regardless
{
"internal_order": [
"flare-log-level.trace",
"flare-log-level.debug",
"flare-log-level.info",
"flare-log-level.warn",
"flare-log-level.error",
"flare-log-level.critical",
"flare-log-level.off",
],
"order": [],
},
{
"action": "enable",
"service_target": {"service": None, "env": None},
"lib_config": cfg,
}
},
],
}

Expand Down Expand Up @@ -182,7 +196,7 @@ def test_settings_missing_lib_config(config, monkeypatch):
base_rc_config = _base_rc_config({})

# Delete "lib_config" from the remote config
del base_rc_config["config"][0]["lib_config"]
del base_rc_config["config"][1]["lib_config"]
assert "lib_config" not in base_rc_config["config"][0]

config._handle_remoteconfig(base_rc_config, None)
Expand Down

0 comments on commit 69f91ee

Please sign in to comment.