-
Notifications
You must be signed in to change notification settings - Fork 347
Report stats span collapses over telemetry (client-side stats) #12070
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
Open
dougqh
wants to merge
6
commits into
master
Choose a base branch
from
dougqh/stats-collapsed-spans-telemetry
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8250653
Report stats span collapses over telemetry (stats.collapsed_spans)
dougqh 56a0836
Hoist stats whole_key collapse counter off the per-dropped-span map l…
dougqh 01daa8b
Test whole_key collapse increments the pre-created StatsMetrics counter
dougqh 6431932
Guard stats telemetry drain against delta loss when the queue is full
dougqh c0ce934
Collect bounded stats collapse counters before the unbounded span reg…
dougqh 869b8b7
Drain StatsMetrics singleton deltas after each test
dougqh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
111 changes: 111 additions & 0 deletions
111
internal-api/src/main/java/datadog/trace/api/metrics/StatsMetrics.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| package datadog.trace.api.metrics; | ||
|
|
||
| import java.util.Collection; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.ConcurrentMap; | ||
| import java.util.concurrent.atomic.AtomicLong; | ||
|
|
||
| /** | ||
| * Telemetry counters for client-side trace-stats span collapses. Mirrors the statsd {@code | ||
| * datadog.tracer.stats.collapsed_spans} metric so the same signal is visible over telemetry, which | ||
| * (unlike statsd) is always wired regardless of whether a dogstatsd sink is configured -- the stats | ||
| * aggregator's {@code HealthMetrics} is {@code NO_OP} when health metrics are disabled. | ||
| * | ||
| * <p>Each collapse "reason" (e.g. {@code collapsed:additional_metric_tags}, {@code | ||
| * oversized:additional_metric_tags}, {@code collapsed:peer_tags}, {@code collapsed:<field>}, {@code | ||
| * collapsed:whole_key}) is a distinct telemetry tag on a single {@code stats.collapsed_spans} | ||
| * counter. The reason set is bounded and low-cardinality by construction (the cardinality limits | ||
| * themselves guarantee it), so a dynamic map keyed by reason cannot itself blow up. | ||
| * | ||
| * <p>Counters are incremented from the single stats-aggregator thread and drained from the | ||
| * telemetry thread: the backing map is a {@link ConcurrentMap} and each counter is an {@link | ||
| * AtomicLong}, so neither side needs external synchronization. {@link | ||
| * TaggedCounter#getValueAndReset()} is only called from the draining thread. | ||
| */ | ||
| public final class StatsMetrics { | ||
| static final String COLLAPSED_SPANS = "stats.collapsed_spans"; | ||
| static final String COLLAPSED_WHOLE_KEY = "collapsed:whole_key"; | ||
|
|
||
| private static final StatsMetrics INSTANCE = new StatsMetrics(); | ||
|
|
||
| // reason tag (e.g. "collapsed:additional_metric_tags") -> counter. Created on first collapse for | ||
| // that reason; the reason set is bounded, so this never grows unboundedly. | ||
| private final ConcurrentMap<String, TaggedCounter> collapsedByReason = new ConcurrentHashMap<>(); | ||
|
|
||
| // The one reason counted per dropped span on the hot aggregator path (aggregate table at cap); | ||
| // every other reason is batched once per reporting cycle. Pre-created and cached so the per-span | ||
| // increment is a direct counter hit rather than a map lookup -- this matters precisely when a | ||
| // cardinality explosion pins the table at cap and every arriving span is dropped, turning a cold | ||
| // path hot. Still registered in the map above, so the telemetry drain sees it with the rest. | ||
| private final TaggedCounter wholeKeyCollapses = | ||
| this.collapsedByReason.computeIfAbsent( | ||
| COLLAPSED_WHOLE_KEY, tag -> new TaggedCounter(COLLAPSED_SPANS, tag)); | ||
|
|
||
| public static StatsMetrics getInstance() { | ||
| return INSTANCE; | ||
| } | ||
|
|
||
| private StatsMetrics() {} | ||
|
|
||
| /** | ||
| * Records {@code count} spans collapsed under the given {@code reason} tag (e.g. {@code | ||
| * collapsed:additional_metric_tags}). No-op for a non-positive count. | ||
| */ | ||
| public void onCollapsedSpans(String reason, long count) { | ||
| if (count <= 0) { | ||
| return; | ||
| } | ||
| collapsedByReason | ||
| .computeIfAbsent(reason, tag -> new TaggedCounter(COLLAPSED_SPANS, tag)) | ||
| .counter | ||
| .addAndGet(count); | ||
| } | ||
|
|
||
| /** | ||
| * Records a single whole-key collapse: a span dropped because the aggregate table was at cap with | ||
| * no entry to evict. Increments the pre-created {@link #COLLAPSED_WHOLE_KEY} counter directly, | ||
| * keeping the per-dropped-span aggregator path off the reason map. | ||
| */ | ||
| public void onWholeKeyCollapse() { | ||
| this.wholeKeyCollapses.counter.addAndGet(1); | ||
| } | ||
|
|
||
| public Collection<TaggedCounter> getTaggedCounters() { | ||
| return this.collapsedByReason.values(); | ||
| } | ||
|
|
||
| /** A named, single-tag counter drained as a telemetry {@code count} metric. */ | ||
| public static final class TaggedCounter implements CoreCounter { | ||
| private final String name; | ||
| private final String tag; | ||
| private final AtomicLong counter = new AtomicLong(); | ||
| private long previousCount; | ||
|
|
||
| TaggedCounter(String name, String tag) { | ||
| this.name = name; | ||
| this.tag = tag; | ||
| } | ||
|
|
||
| @Override | ||
| public String getName() { | ||
| return this.name; | ||
| } | ||
|
|
||
| public String getTag() { | ||
| return this.tag; | ||
| } | ||
|
|
||
| @Override | ||
| public long getValue() { | ||
| return this.counter.get(); | ||
| } | ||
|
|
||
| @Override | ||
| public long getValueAndReset() { | ||
| long count = this.counter.get(); | ||
| long delta = count - this.previousCount; | ||
| this.previousCount = count; | ||
| return delta; | ||
| } | ||
| } | ||
| } |
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
122 changes: 122 additions & 0 deletions
122
internal-api/src/test/java/datadog/trace/api/metrics/StatsMetricsTest.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| package datadog.trace.api.metrics; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import java.util.Map; | ||
| import java.util.function.Function; | ||
| import java.util.stream.Collectors; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| /** | ||
| * Unit tests for {@link StatsMetrics}. The instance is a process-wide singleton, so each test uses | ||
| * its own uniquely-named collapse reasons to stay isolated from other tests' counters. | ||
| */ | ||
| class StatsMetricsTest { | ||
|
|
||
| private static Map<String, StatsMetrics.TaggedCounter> countersByTag() { | ||
| return StatsMetrics.getInstance().getTaggedCounters().stream() | ||
| .collect(Collectors.toMap(StatsMetrics.TaggedCounter::getTag, Function.identity())); | ||
| } | ||
|
|
||
| @AfterEach | ||
| void drainDeltas() { | ||
| // StatsMetrics is a process-wide singleton; these tests leave per-reason deltas behind. Drain | ||
| // every counter so they do not leak into another collector's expectations when tests share a | ||
| // Gradle worker (e.g. CoreMetricCollectorTest asserting an exact span-metric count). | ||
| for (StatsMetrics.TaggedCounter counter : StatsMetrics.getInstance().getTaggedCounters()) { | ||
| counter.getValueAndReset(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void accumulatesPerReasonAndEmitsResetDeltas() { | ||
| StatsMetrics metrics = StatsMetrics.getInstance(); | ||
| String reason = "collapsed:test_accumulate"; | ||
|
|
||
| metrics.onCollapsedSpans(reason, 3); | ||
| metrics.onCollapsedSpans(reason, 4); | ||
|
|
||
| StatsMetrics.TaggedCounter counter = countersByTag().get(reason); | ||
| assertEquals(StatsMetrics.COLLAPSED_SPANS, counter.getName()); | ||
| assertEquals(reason, counter.getTag()); | ||
| assertEquals(7, counter.getValue(), "getValue reports the running total"); | ||
|
|
||
| // First drain returns the whole accumulated delta; a second drain with no activity returns 0. | ||
| assertEquals(7, counter.getValueAndReset(), "first drain returns the accumulated delta"); | ||
| assertEquals(0, counter.getValueAndReset(), "no new activity -> zero delta"); | ||
|
|
||
| metrics.onCollapsedSpans(reason, 5); | ||
| assertEquals(5, counter.getValueAndReset(), "only the post-drain increment is returned"); | ||
| } | ||
|
|
||
| @Test | ||
| void separateReasonsGetSeparateCounters() { | ||
| StatsMetrics metrics = StatsMetrics.getInstance(); | ||
| String collapsed = "collapsed:test_separate"; | ||
| String oversized = "oversized:test_separate"; | ||
|
|
||
| metrics.onCollapsedSpans(collapsed, 2); | ||
| metrics.onCollapsedSpans(oversized, 9); | ||
|
dougqh marked this conversation as resolved.
|
||
|
|
||
| Map<String, StatsMetrics.TaggedCounter> counters = countersByTag(); | ||
| assertEquals(2, counters.get(collapsed).getValue()); | ||
| assertEquals(9, counters.get(oversized).getValue()); | ||
| } | ||
|
|
||
| @Test | ||
| void nonPositiveCountsAreIgnored() { | ||
| StatsMetrics metrics = StatsMetrics.getInstance(); | ||
| String reason = "collapsed:test_nonpositive"; | ||
|
|
||
| metrics.onCollapsedSpans(reason, 0); | ||
| metrics.onCollapsedSpans(reason, -5); | ||
|
|
||
| // No counter is created for a reason that never saw a positive count. | ||
| assertNull(countersByTag().get(reason), "no counter created for non-positive counts"); | ||
|
|
||
| metrics.onCollapsedSpans(reason, 4); | ||
| assertEquals(4, countersByTag().get(reason).getValue()); | ||
| // A later non-positive count leaves the running total untouched. | ||
| metrics.onCollapsedSpans(reason, -1); | ||
| assertEquals(4, countersByTag().get(reason).getValue()); | ||
| } | ||
|
|
||
| @Test | ||
| void wholeKeyCollapseIncrementsPreCreatedCounter() { | ||
| StatsMetrics metrics = StatsMetrics.getInstance(); | ||
|
|
||
| // The whole_key counter is pre-created at construction, so it is always present in the drain. | ||
| StatsMetrics.TaggedCounter counter = countersByTag().get(StatsMetrics.COLLAPSED_WHOLE_KEY); | ||
| assertEquals(StatsMetrics.COLLAPSED_SPANS, counter.getName()); | ||
| assertEquals(StatsMetrics.COLLAPSED_WHOLE_KEY, counter.getTag()); | ||
|
|
||
| long before = counter.getValue(); | ||
| metrics.onWholeKeyCollapse(); | ||
| metrics.onWholeKeyCollapse(); | ||
| assertEquals(before + 2, counter.getValue(), "each call increments the counter by one"); | ||
|
|
||
| // Routing the same tag through onCollapsedSpans hits the same pre-created counter instance. | ||
| assertTrue( | ||
| countersByTag().get(StatsMetrics.COLLAPSED_WHOLE_KEY) == counter, | ||
| "whole_key resolves to a single stable counter"); | ||
| metrics.onCollapsedSpans(StatsMetrics.COLLAPSED_WHOLE_KEY, 3); | ||
| assertEquals(before + 5, counter.getValue()); | ||
| } | ||
|
|
||
| @Test | ||
| void reasonCounterIsStableAcrossLookups() { | ||
| StatsMetrics metrics = StatsMetrics.getInstance(); | ||
| String reason = "collapsed:test_stable"; | ||
|
|
||
| metrics.onCollapsedSpans(reason, 1); | ||
| StatsMetrics.TaggedCounter first = countersByTag().get(reason); | ||
| metrics.onCollapsedSpans(reason, 1); | ||
| StatsMetrics.TaggedCounter second = countersByTag().get(reason); | ||
|
|
||
| assertTrue(first == second, "same reason maps to the same counter instance"); | ||
| assertEquals(2, first.getValue()); | ||
| } | ||
| } | ||
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.
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.
question: Why method are named differently, this raises question when reading the code, yet, it seems the sgtats are emitted on the same "conditions".
I suggest aligning method names, unless there's a valid reason not to.
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.
Fair point — juxtaposed like this they do read a little oddly. The names differ on purpose: each mirrors the vocabulary of the sink it feeds, and the two sinks are named independently.
HealthMetrics.onTagCardinalityBlocked(...)is one of HealthMetrics'onXevent callbacks; that subsystem's vocabulary is "blocked" (the offending tag value is blocked/collapsed to the sentinel), and its statsd tags arecollapsed:/oversized:.StatsMetrics.onCollapsedSpans(...)mirrors the telemetry metric it increments,stats.collapsed_spans— the collapse reason rides in the tag rather than the method name.So both fire under the same condition because they report the same underlying event to two sinks, but renaming either to match the other would make that method name diverge from the name of the thing it actually reports. I'd rather keep each aligned to its own metric/event name.
Open to a different split if you feel strongly — but I don't think there's a single verb that reads naturally against both
stats.collapsed_spansand the health*_blockedfamily at once.