fix(telemetry): app-extended-heartbeat event not correctly reporting entire config state#4633
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
|
✨ Fix all issues with BitsAI or with Cursor
|
a432849 to
6e55261
Compare
6e55261 to
da56468
Compare
genesor
left a comment
There was a problem hiding this comment.
I did not gave a deep review of the entire flow but I left two questions.
Also there is no test modifications in internal/mapper is this expected ?
da56468 to
87324f8
Compare
BenchmarksBenchmark execution time: 2026-04-08 18:52:01 Comparing candidate commit 45d9d7d in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 216 metrics, 8 unstable metrics.
|
08369d3 to
32ffc6a
Compare
|
Hey @genesor I ended up scrapping the re-injection approach entirely. The updated design has heartbeatEnricher pull the full config state directly from the configuration data source via a getConfigs() function passed at construction to be called before reporting extended heartbeat, no pipeline workarounds needed The config store now retains all registered configs (with a pending set for deltas) and exposes an All() snapshot method |
32ffc6a to
c2e19b6
Compare
genesor
left a comment
There was a problem hiding this comment.
Nice work Ayan, I think we should have better comments in the code explaining the flow and how it will now work.
Before we were draining a struct and be done with it. It's not the case anymore, the flow should be explained somewhere so we don't have to re-understand it everytime 👍
| config map[configKey]transport.ConfKeyValue | ||
| pending map[configKey]struct{} |
There was a problem hiding this comment.
It would be great to have comments on config and pending to explain how they differ and how they are used.
|
|
||
| configs := make([]transport.ConfKeyValue, 0, len(c.config)) | ||
| for _, conf := range c.config { | ||
| conf.Value = SanitizeConfigValue(conf.Value) |
There was a problem hiding this comment.
Would it useful to have the same default handling for SeqID and Origin here as well ? Payload sets some values, depending on the calling order could we face some issues ?
| } | ||
| } | ||
|
|
||
| // All returns a sanitized snapshot of all accumulated configs. |
There was a problem hiding this comment.
This comment should explain how this function is used.
cee6f48 to
adf39b0
Compare
828d315 to
ff8ff8f
Compare
ff8ff8f to
5bbf1ab
Compare
… of pipeline accumulation Replace the push-based config accumulation in heartbeatEnricher with a pull model that reads the full config state on demand from the configuration data source. Previously, heartbeatEnricher accumulated configs by watching AppClientConfigurationChange and AppStarted payloads flow through the pipeline. This was broken because appStartedReducer consumed the initial config payload before heartbeatEnricher could see it. Now, the configuration data source retains all registered configs (using a pending set to track deltas for Payload()) and exposes an All() method. heartbeatEnricher calls getConfigs() when emitting the extended heartbeat to get the full current state directly from the source of truth. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5bbf1ab to
a7cfb7a
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
This pull request is not mergeable according to GitHub. Common reasons include pending required checks, missing approvals, or merge conflicts — but it could also be blocked by other repository rules or settings.
The expected merge time in
|
What does this PR do?
Replace the push-based config accumulation in
heartbeatEnricherwith a pull model that reads the full config state on demand from theconfigurationdata source.Problem:
heartbeatEnricherhad anAppStartedcase to capture startup configs, but it was unreachable becauseappStartedReducerconsumed theAppStartedpayload beforeheartbeatEnrichercould see it. The extended heartbeat was missing all startup configurations.Solution: The
configurationdata source now retains all registered configs (using apendingset to track deltas forPayload()) and exposes anAll()method.heartbeatEnrichercallsgetConfigs()when emitting the extended heartbeat to get the full current state directly from the source of truth.Changes:
configuration.go— Single map withpendingset.Add()writes to config and marks pending.Payload()extracts only pending keys. NewAll()returns full accumulated state.internal/mapper/default.go—NewDefaultMapperacceptsfunc() []transport.ConfKeyValue.heartbeatEnricherpulls configs on demand instead of accumulating from pipeline. RemovedAppStartedcase.client.go— Passesconfiguration.AlltoNewDefaultMapper.client_test.go— Added integration tests for extended heartbeat config inclusion (single, multiple, dedup).Motivation
Discovered while adding
app-extended-heartbeatsystem tests in DataDog/system-tests#6338. The extended heartbeat was missing startup configs becauseappStartedReducerconsumed them beforeheartbeatEnrichercould see them. Rather than work around the pipeline ordering issue, this PR fixes the root cause by havingheartbeatEnricherread config state directly from the source of truth.API spec for reference: https://github.com/DataDog/instrumentation-telemetry-api-docs/blob/main/GeneratedDocumentation/ApiDocs/v2/SchemaDocumentation/Schemas/app_extended_heartbeat.md