Opt-in client-side aggregation and some Datadog client parity changes#321
Open
bdeitte wants to merge 43 commits into
Open
Opt-in client-side aggregation and some Datadog client parity changes#321bdeitte wants to merge 43 commits into
bdeitte wants to merge 43 commits into
Conversation
- flush() and close() now wait for in-flight aggregator/unbuffered sends to drain across all involved clients (incl. children), fixing early callbacks and dropped child-recorded metrics on parent close - DD_DOGSTATSD_URL without a port now defaults to 8125 so it stays authoritative over DD_DOGSTATSD_PORT - datadogMode test env cleanup derived from constants.DATADOG_SIGNAL_ENV_VARS - README datadog auto-detection list adds DD_TAGS/DD_DOGSTATSD_URL/SOCKET and clarifies DATADOG_TAGS is not a signal - types.d.mts re-exports AggregationOptions for ESM consumers
Fix correctness issues found reviewing the DD_TAGS, DD_DOGSTATSD_URL/SOCKET, flush() and client-side aggregation work: - close() force-close now resolves pending drain promises so a concurrent flush(callback) is not orphaned - flush() guards a throwing aggregator flush so the callback still fires - post-close records are no longer silently aggregated into a window that never flushes (aggregator marked closed) - a client-level default sampleRate < 1 no longer disables aggregation - interval-driven aggregator flushes track routed clients so close() drains them - aggregation.flushInterval is validated like bufferFlushInterval - invalid DD_DOGSTATSD_URL falls back to DD_DOGSTATSD_SOCKET - DD_TAGS no longer clobbers a child client's explicit globalTags - aggregation contextKey includes default cardinality / container id / external data and normalizes object-tag key order; NaN values are not aggregated - DD_DOGSTATSD_URL/SOCKET env transport and aggregation are disabled for telegraf - test cleanup list includes DATADOG_TAGS Adds tests for each fix; updates README and CHANGES.
- flush(callback) now waits on the aggregator's active clients, so a child
send started by an interval-driven flush is not skipped
- replace the permanent routedClients Set with a self-pruning activeClients
set (tracked only while a send is in flight) to avoid unbounded retention of
dynamically-created child clients
- setupDatadogGlobalTags skips reapplying only DD_TAGS for children; the DD_*
mapping still applies so DD_ENV/DD_SERVICE/DD_VERSION/DD_ENTITY_ID keep their
documented precedence over a child's globalTags
- aggregation contextKey encodes object-tag value types so { a: undefined } and
{ a: null } no longer collide into one context
Adds/updates tests for each; updates CHANGES.
JSON.stringify collapses NaN, Infinity and -Infinity all to null, so object tags with those values (emitted as a:NaN, a:Infinity, a:-Infinity) collapsed into one aggregation context, dropping or mislabeling a series. Encode each value via String() alongside its type instead. Addresses #929 review finding.
The fake-timer flush drain tests checked sendDrained only after clock.tickAsync(30) resolved, by which point the deferred send has always drained. That made the assertion unconditionally true, so an early-firing flush callback would not be caught. Move the assertion into the flush callback and await it alongside tickAsync via Promise.all so a regression fails the test. Verified by injecting an immediate-callback regression: all three tests fail as expected.
The telegraf exclusion in setupAggregation was convention-only (mirroring the DogStatsD-only event/check/telemetry pattern), not a technical limit: counts/gauges/sets share an identical name:value|type wire body across StatsD, DogStatsD and Telegraf, and telegraf tag formatting still applies at send time via the normal path. Since plain StatsD aggregation was already allowed, excluding only telegraf was inconsistent. Remove the guard, replace the disable-for-telegraf test with one asserting aggregation works and emits telegraf's inline tag format, and update README/CHANGES to say aggregation works for all client types.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds opt-in client-side aggregation and a public flush() API to improve parity with official DogStatsD clients and improve behavior in short-lived/serverless environments, while also extending Datadog env-var support for tags and transport configuration.
Changes:
- Add optional client-side aggregation for counts/gauges/sets with configurable flush interval and context cap.
- Add
flush([callback])to proactively send buffered + aggregated metrics and wait for in-flight routed sends when a callback is provided. - Add DD_TAGS/DATADOG_TAGS parsing for global tags and DD_DOGSTATSD_URL/DD_DOGSTATSD_SOCKET env transport configuration (+ related tests/docs), plus dev dependency lockfile updates.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| types.d.ts | Add AggregationOptions, aggregation option, and flush() typing. |
| types.d.mts | Re-export AggregationOptions for ESM typings. |
| lib/statsd.js | Implement aggregation integration, flush(), env transport precedence, and close/drain behavior updates. |
| lib/helpers.js | Add parseDogstatsdUrl() and getDogstatsdEnvTransport() helpers. |
| lib/constants.js | Extend Datadog signal env-var list to include DD_TAGS and env transport vars. |
| lib/aggregator.js | New aggregation implementation (context keying, cap overflow behavior, flush routing, active-client tracking). |
| README.md | Document aggregation/flush APIs and DD_TAGS + env transport configuration/precedence. |
| CHANGES.md | Add Unreleased changelog entries for the new features and lockfile updates. |
| CLAUDE.md | Document the new aggregator module in repo structure notes. |
| test/init.js | Add coverage for env transport precedence and telegraf exclusion. |
| test/helpers.js | Add unit tests for URL parsing and env transport resolution helpers. |
| test/globalTags.js | Add coverage for DD_TAGS/DATADOG_TAGS behavior and precedence with child clients. |
| test/flush.js | Add coverage for new flush() behavior including in-flight sends and error isolation. |
| test/envTransport.js | Ensure useDefaultRoute prevents DD env transport from altering routing. |
| test/datadogMode.js | Keep env cleanup list in sync by deriving from constants. |
| test/aggregation.js | Extensive aggregation behavior and edge-case coverage. |
| package-lock.json | Update lockfile to address dev dependency security advisories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…s collision
Telegraf emits ['route:a'] and ['route=a'] identically as route=a, but the
aggregator keyed them on the raw caller array as two contexts. A gauge series
mixing both forms then flushed in creation order rather than update order and
settled on a stale value.
Key on the canonical emitted form via the same formatTags path send() uses,
plus telegraf's :->= rewrite. This also folds together the pre-existing
array-vs-object aliasing (['a:1'] and {a:1} both emit a:1) while preserving the
object-value String() semantics (undefined/null and NaN/Infinity stay distinct).
Reported in review #967.
These three tests assert exact wire output in datadog mode but left origin detection on. On the Linux CI runner cgroups yield a container ID, appending an unexpected |c:<id> field and failing the assertions; on macOS origin detection is a no-op so they passed locally. Set originDetection:false to match the deterministic pattern already used in globalTags.js and datadogMode.js.
…nterval label - getDogstatsdEnvTransport now trims DD_DOGSTATSD_SOCKET and rejects an empty/whitespace-only path with a warning, instead of silently routing to an invalid socket (mirrors the URL parser's missing-path validation). Adds tests. - validateInterval label for the aggregation flush interval is now 'aggregation.flushInterval', matching the documented option path.
clientContextSuffix still keyed global tags on globalTags.join(','), which is
not the emitted form for Telegraf. Two child clients sharing an aggregator with
global tags that differ only in a wire-invisible way — e.g. Telegraf ['route:a']
vs ['route=a'], both emitting route=a — split into separate gauge contexts that
flushed in creation order and settled on a stale value.
Reuse canonicalTagsKey for the global-tags portion of the suffix so it matches
the per-call tag keying from #967. Adds a child/global-tag telegraf gauge
regression test.
Owner
Author
|
I need to do some more reviewing of this, which likely won't happen until Friday or later, and then this can get merged as a major version update given the number of changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The big change here is a opt-in client-side aggregation of counts, gauges and sets via the
aggregationoption, for better parity with official DogStatsD clients but also for use with any StatsD, DogStatsD or Telegraf client. This includes an aggregationmaxContextsoption (default 5000) bounding the number of live aggregation contexts; new contexts beyond the cap are sent directly with a one-time warning.A change to help with the aggregation is a a public
flush()method to send buffered metrics (and any pending aggregated metrics) immediately, useful for serverless and other short-lived environmentsThe above started when looking at official Datadog client parity, but the aggregation changes widened. Two changes though for that:
And since it showed up while working on these changes, fix dev-dependency security advisories (@babel/core, js-yaml)