Add StatsConcentrator for time-bucketed span aggregation#2871
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6a83a9e71
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04ead6a61d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04ead6a61d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
tienquocbui
force-pushed
the
kelvin.bui/css-stats-concentrator
branch
from
2119ad8 to
af06d58
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af06d58af8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for bucket in exportedBuckets { | ||
| writer.write(value: bucket) |
There was a problem hiding this comment.
When a flush returns more than one bucket, this loop writes each bucket as a separate storage event, but StatsRequestBuilder.request(for:) builds the upload body by raw-concatenating Event.data with no delimiter or array wrapper. A normal 30s flush can export multiple 10s buckets, so the request body becomes adjacent JSON objects like {...}{...}, which is not valid application/json and causes the whole stats upload for that batch to be rejected; write a single encoded stats payload/batch instead of one event per bucket.
Useful? React with 👍 / 👎.
tienquocbui
force-pushed
the
kelvin.bui/css-stats-concentrator
branch
from
af06d58 to
2a2b5cb
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for a06bebf: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: rejected
and removed
mergequeue-status: in_progress
labels
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
Build pipeline has failing jobs for 9ed7ead: What to do next?
DetailsSince those jobs are not marked as being allowed to fail, the pipeline will most likely fail. |
tienquocbui
force-pushed
the
feature/client-side-stats
branch
from
bb6e850 to
d3bd54a
Compare
- Replace @ReadWriteLock with serial DispatchQueue so add() never blocks the caller and flush() drains pending work synchronously - Move fnv64a and stochasticRound into StatsUtils enum namespace - Simplify flush cutoff using force ternary, document bufferLen - Improve doc comments on flushStats(force:) and stochasticRound - Use weak capture for stats in onSpanFinished closure
tienquocbui
force-pushed
the
kelvin.bui/css-stats-concentrator
branch
from
2a2b5cb to
3e58d7b
Compare
gh-worker-dd-mergequeue-cf854d
Bot
merged commit e061102
into
feature/client-side-stats
gh-worker-dd-mergequeue-cf854d
Bot
deleted the
kelvin.bui/css-stats-concentrator
branch
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
3 participants
What and why?
Implements the
StatsConcentrator, the core aggregation engine for client-side stats. This is the component that receives span snapshots from theonSpanFinishedcallback and groups them into time-bucketed stats (hit counts, error counts, duration totals) keyed by aggregation dimensions (service, operation, resource, HTTP status, span kind, etc.).This PR also wires the concentrator into the existing
ClientStatsFeaturewith periodic timer-based flushing andFlushableconformance for SDK teardown.How?
New types (
StatsConcentrator.swift):StatsConcentrator: aggregatesSpanSnapshots into 10-second time buckets, matching the Go reference implementation. Uses@ReadWriteLockfor thread safety. Supports eligibility filtering (top-level, measured, or qualifying span kind),oldestTstracking to prevent late-arriving spans from creating already-flushed buckets, and periodic + force flush.AggregationKey: the set of dimensions by which spans are grouped (service, operation, resource, HTTP status, type, span kind, is_trace_root, peer tags hash, service source).Trilean: matches the protobuf enum foris_trace_root(notSet / true / false).GroupedStats,StatsBucket: internal mutable types for running counters within a bucket.ExportedBucket,ExportedGroupedStats: immutableEncodable+Sendablestructs for serialization.fnv64a(): FNV-64a hash for peer tag aggregation keys, matching Go'stagsFnvHash.stochasticRound(): converts floating-point counts to UInt64 (effectively truncation since weight is always 1.0 for now).Wiring changes:
ClientStatsFeaturenow holds aStatsConcentrator, runs a 30-second flush timer, conforms toFlushable, and writes exported buckets to feature storage viaeventWriteContext.Trace.enableOrThrow()wirestrace.tracer.onSpanFinishedtostats.concentrator.add(snapshot)and passesconfiguration.dateProviderto ensure the stats pipeline shares the same clock as the tracer.SpanSnapshotgains atypefield (populated fromtags["span.type"]or defaulting to"custom"inDDSpan.createSnapshot()).SpanSnapshot.mockAny()/.mockWith()helpers added toTestUtilities.Known limitations (to be addressed in subsequent PRs):
okSummary/errorSummaryareData()placeholders; DDSketch implementation is next.StatsRequestBuilderstill concatenates raw event bytes withapplication/jsoncontent type. Multi-bucket uploads will produce invalid JSON. The real MessagePack encoder replaces this in a later PR.Trace.Configuration.serviceis nil,snapshot.serviceresolves to""instead of falling back toDatadogContext.service(which is only available asynchronously viaeventWriteContext). This means stats may be grouped under an empty service name for default-config users. A proper fix requires resolving the core context service at snapshot creation or aggregation time, tracked for a follow-up PR.Review checklist
make api-surfacewhen adding new APIs