[RUM-3572] Replace metrics sampling with call site sampling#3025
[RUM-3572] Replace metrics sampling with call site sampling#3025saraSr5 wants to merge 10 commits into
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: 76e829ed80
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| guard Sampler(samplingRate: sampleRate).sample() else { | ||
| return | ||
| } |
There was a problem hiding this comment.
When DataUploadWorker emits UploadQualityMetric it calls telemetry.metric without a sampleRate (for example in DatadogCore/Sources/Core/Upload/DataUploadWorker.swift:272 and :281), and RUM’s TelemetryInterceptor consumes those .metric messages from the bus to aggregate rse.upload_quality (DatadogRUM/Sources/Integrations/TelemetryInterceptor.swift:23). This new guard drops the default 85% at the call site before the message bus, so most upload-quality cycles never reach the interceptor and the Session Ended metric undercounts upload failures/blockers; previously the receiver-side sampling happened after this interception path.
Useful? React with 👍 / 👎.
saraSr5
changed the title
saraSr5
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35d6dd55ba
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| guard Sampler(samplingRate: sampleRate).sample() else { | ||
| return | ||
| } |
There was a problem hiding this comment.
With sampling now inside the Telemetry.metric extension, every conformer that relies on the default implementation, including TestUtilities.TelemetryMock, randomly discards metrics before tests can observe them. Existing tests still assert default-rate metrics, e.g. DatadogCore/Tests/Datadog/Core/Persistence/FilesOrchestrator+MetricsTests.swift uses 1.5% batch metrics and DatadogProfiling/Tests/SDKMetrics/ProfilingTelemetryControllerTests.swift uses the 20% default, so CI will fail/flap unless those call sites are made deterministic or the sampling dependency is injectable/bypassed in tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a good point, our tests will be become flaky with the head based sampling we are introducing here. I am not sure this can be solved with this design 🤔
| guard Sampler(samplingRate: sampleRate).sample() else { | ||
| return | ||
| } |
There was a problem hiding this comment.
Moving usage sampling into Telemetry.usage means components that emit default 15% usage telemetry no longer always send a message through test cores. DatadogWebViewTracking/Tests/WebViewTrackingTests.swift asserts .trackWebView is sent exactly once/again after disable, but WebViewTracking calls core.telemetry.usage(event: .trackWebView) with the default rate, so those tests now fail about 85% of the time unless the emission path can force 100% or inject deterministic sampling.
Useful? React with 👍 / 👎.
| case let .metric(metric): | ||
| if sampled(event: metric) { | ||
| send(metric: metric) | ||
| } | ||
| send(metric: metric) | ||
| case .usage(let usage): | ||
| if sampled(event: usage) { | ||
| send(usage: usage) | ||
| } | ||
| send(usage: usage) |
There was a problem hiding this comment.
When a caller sends a raw TelemetryMessage through Telemetry.send(telemetry:), such as .usage(.init(..., sampleRate: 0)) or a forwarded .metric, these branches now write it unconditionally because the only sampling guard lives in the convenience methods. Before this change the receiver honored each message's sampleRate after it reached the bus, so raw messages with sampleRate: 0 are now emitted with effectiveSampleRate 0 instead of being dropped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is acceptable, head base sampling makes the sampleRate obsolete.
| _ metric: MethodCalledTrace?, | ||
| isSuccessful: Bool = true, | ||
| tailSampleRate: SampleRate = MetricTelemetry.defaultSampleRate | ||
| callSiteSampleRate: SampleRate = MetricTelemetry.defaultSampleRate |
There was a problem hiding this comment.
Renaming the public argument label from tailSampleRate: to callSiteSampleRate: breaks any downstream integrator or cross-platform wrapper that already calls this telemetry helper with the old label. Since the semantic change can be implemented behind the same API, keep a compatibility overload or deprecate the old label instead of removing it in-place.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| public struct MetricTelemetry: SampledTelemetry { | ||
| public struct MetricTelemetry { |
There was a problem hiding this comment.
MetricTelemetry and UsageTelemetry are public types and SampledTelemetry remains public, so removing these conformances is a source and runtime compatibility break for downstream code that stores or filters sampled telemetry through that protocol. The new receiver implementation no longer needs the protocol, but leaving the conformances in place would preserve compatibility without changing sampling behavior.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 548ed6e200
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| case let .metric(metric): | ||
| if sampled(event: metric) { | ||
| send(metric: metric) | ||
| } | ||
| send(metric: metric) | ||
| case .usage(let usage): | ||
| if sampled(event: usage) { | ||
| send(usage: usage) | ||
| } | ||
| send(usage: usage) |
There was a problem hiding this comment.
When a caller uses the still-available Telemetry.send(telemetry:) path with a .metric or .usage message, this receiver now writes the event without checking its per-event sampleRate; for example, a raw .metric(..., sampleRate: 0) will pass through here and can be uploaded/count toward the session limit, whereas the removed sampled(event:) guard rejected it. Since MetricTelemetry and UsageTelemetry still carry sample-rate semantics independently of the convenience metric()/usage() helpers, keep sampling on this raw-message path or apply it before forwarding to the bus.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good point. This is a behavior change for callers that bypass the convenience metric() / usage() helpers and send raw .metric / .usage telemetry directly. I checked production call sites and couldn't see the case where we send raw metric/usage telemetry this way, all production emissions go through the convenience helpers where call-site sampling is applied. Reintroducing receiver-side sampling as is would double sample the normal helper path, so I'd prefer not to add it back there.
Given that this is currently an edge case rather than an observed production path, I propose keeping this PR scoped to the call site sampling migration and tracking raw send(telemetry:) sampling separately if we want to preserve that contract explicitly.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 548ed6e200
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| UploadQualityMetric.track: featureName | ||
| ] | ||
| ], | ||
| sampleRate: .maxSampleRate // must always reach TelemetryInterceptor for Session Ended aggregation |
There was a problem hiding this comment.
This line (and the identical failure path below) changes upload_quality from the default metric sample rate to 100%, but that same sampleRate is still used by TelemetryReceiver.send(metric:) to compute and upload the debug metric. In normal RUM runs this means standalone upload_quality telemetry is now gated only by the global telemetry sampler (20% by default) instead of the previous 15% metric sampling after TelemetryInterceptor had already aggregated it, increasing uploaded volume/counts by about 6.7x; keep the bus/interceptor path unsampled while preserving the receiver-side/default metric sampling for the debug metric.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I checked the actual RUM receiver chain and the global MessageBus calls each registered feature receiver, but RUM registers a CombinedFeatureMessageReceiver. Inside RUM, TelemetryInterceptor is before
TelemetryReceiver, and CombinedFeatureMessageReceiver uses contains(where:), so it stops after TelemetryInterceptor returns true for UploadQualityMetric. Therefore upload_quality is aggregated into Session Ended and is not forwarded to TelemetryReceiver as a debug telemetry event.
saraSr5
force-pushed
the
saraSr5/RUM-3572/call-site-metric-sampling
branch
from
102c400 to
d7e8d94
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7e8d94765
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| import Foundation | ||
| import DatadogInternal | ||
| @testable import DatadogInternal |
There was a problem hiding this comment.
When the TestUtilities library/product is built outside a test-enabled configuration, such as its Package.swift product or the Xcode Release target, DatadogInternal is not compiled with ENABLE_TESTABILITY, so this new @testable import fails with “module 'DatadogInternal' was not compiled for testing” before any tests can run. The new TelemetryMock helpers need a public construction path instead of making the whole TestUtilities target depend on testability.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ae8580319
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| @_spi(Internal) | ||
| public init(name: String, attributes: [String: Encodable], sampleRate: SampleRate = Self.defaultSampleRate) { |
There was a problem hiding this comment.
The unchanged DatadogSessionReplay/Tests/Feature/SessionReplayTelemetryTests.swift imports @testable import DatadogInternal rather than the Internal SPI and constructs .metric(.init(name:attributes:sampleRate:)); adding this explicit SPI-only initializer also removes the synthesized internal memberwise initializer that @testable previously exposed, so the DatadogSessionReplayTests target cannot compile unless this initializer is visible to testable imports or that test imports the SPI.
Useful? React with 👍 / 👎.
| public func metric(name: String, attributes: [String: Encodable], sampleRate: SampleRate = MetricTelemetry.defaultSampleRate) { | ||
| guard Sampler(samplingRate: sampleRate).sample() else { | ||
| return |
There was a problem hiding this comment.
Fresh evidence since the earlier sampling comments: although TelemetryMock now overrides metric/usage, Swift supplies default arguments statically, so a value typed as Telemetry that omits sampleRate resolves to this protocol-extension implementation to get the default and runs the sampling guard before the conformer's override or spy sees the event. Components such as URLSessionRUMResourcesHandler store injected telemetry as Telemetry and call usage without a sample rate, leaving default-rate telemetry tests/custom conformers flaky; use a separate defaulting overload that calls the sampleRate: requirement explicitly.
Useful? React with 👍 / 👎.
…fault-rate telemetry
maxep
left a comment
maxep
left a comment
There was a problem hiding this comment.
Good work! I have some specific feedback on the implementation itself, but I think there is a wider issue and it's testability: The Sampler decision now reside in the Telemetry protocol extension, which prevent any injection and predictability in tests. It would be best if we could have some control in tests about the sampler and/or sample-rate.
Maybe the solution would be to give sampling control to the concrete Telemetry implementation
| /// This sample rate is applied in the telemetry receiver, after the metric has been processed by the SDK core (tail-based sampling). | ||
| public let sampleRate: SampleRate | ||
|
|
||
| @_spi(Internal) |
There was a problem hiding this comment.
nit/ None of the public api here are @_spi(Internal), it is already part of DatadogInternal module
| @_spi(Internal) |
| /// Collects a metric value. The default implementation applies call-site sampling before sending. | ||
| func metric(name: String, attributes: [String: Encodable], sampleRate: SampleRate) | ||
| /// Collects a usage telemetry event. The default implementation applies call-site sampling before sending. | ||
| func usage(event: UsageTelemetry.Event, sampleRate: SampleRate) |
There was a problem hiding this comment.
cr/ This should not be part of the Telemetry abstraction: Telemetry abstraction is about sending TelemetryMessage, and metric/usage are specialized messages with convenient methods in the extension. The abstraction itself declares only the required methods
| public func metric(name: String, attributes: [String: Encodable], sampleRate: SampleRate = MetricTelemetry.defaultSampleRate) { | ||
| public func metric(name: String, attributes: [String: Encodable], sampleRate: SampleRate) { |
There was a problem hiding this comment.
cr/ The default should stay in my opinion, it is more convenient at call site
| case let .metric(metric): | ||
| if sampled(event: metric) { | ||
| send(metric: metric) | ||
| } |
There was a problem hiding this comment.
This removes the need for MetricTelemetry to be SampledTelemetry compliant, as it removes the need for the var sampleRate in the metric
| case .usage(let usage): | ||
| if sampled(event: usage) { | ||
| send(usage: usage) | ||
| } |
There was a problem hiding this comment.
Same here: This removes the need for UsageTelemetry to be SampledTelemetry compliant, as it removes the need for the var sampleRate in the metric
There was a problem hiding this comment.
nit/ These 2 lines can be removed, they are not part of the Telemetry interface
There was a problem hiding this comment.
This will effectively no longer be a tailSampleRate, it will become headSampleRate
| case let .metric(metric): | ||
| if sampled(event: metric) { | ||
| send(metric: metric) | ||
| } | ||
| send(metric: metric) | ||
| case .usage(let usage): | ||
| if sampled(event: usage) { | ||
| send(usage: usage) | ||
| } | ||
| send(usage: usage) |
There was a problem hiding this comment.
This is acceptable, head base sampling makes the sampleRate obsolete.
| guard Sampler(samplingRate: sampleRate).sample() else { | ||
| return | ||
| } |
There was a problem hiding this comment.
This is a good point, our tests will be become flaky with the head based sampling we are introducing here. I am not sure this can be solved with this design 🤔
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aabffc368e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// This sample rate is applied in the telemetry receiver, after the metric has been processed by the SDK core (tail-based sampling). | ||
| public let sampleRate: SampleRate | ||
|
|
||
| public init(name: String, attributes: [String: Encodable], sampleRate: SampleRate = Self.defaultSampleRate) { |
There was a problem hiding this comment.
This line adds a new public initializer to MetricTelemetry, which changes DatadogInternal’s public module interface for any integrator importing that framework; the root AGENTS.md explicitly says not to introduce new public API without RFC review. If this constructor is only needed for SDK internals/tests, keep it internal or expose it via SPI/test-only access instead of making it part of the public API surface.
Useful? React with 👍 / 👎.
| public init() { } | ||
| /// no-op | ||
| public func send(telemetry: TelemetryMessage) { } | ||
| public func startMethodCalled(operationName: String, callerClass: String, samplingRate: Float) -> MethodCalledTrace? { return nil } | ||
| public func stopMethodCalled(_ metric: MethodCalledTrace?, isSuccessful: Bool) { } | ||
| } |
There was a problem hiding this comment.
This change also removes NOPTelemetry.startMethodCalled(operationName:callerClass:samplingRate:), and the protocol extension does not replace it because the remaining call label is headSampleRate:. Any downstream or test-support code that instantiates NOPTelemetry directly and calls the existing samplingRate: no-op overload will stop compiling; keep a compatibility overload that returns nil or forwards to the renamed API.
Useful? React with 👍 / 👎.
2 participants
What and why?
Replaces metric sampling in
TelemetryReceiverwith call site sampling inTelemetry.metric()andTelemetry.usage().Previously, metrics and usage events were sent through the MessageBus regardless of their sample rate, the sampling decision was made inside
TelemetryReceiverafter the event had already been processed by the SDK core. This caused unnecessary processing for events that would ultimately be discarded.Now, the sampling decision is made at the call site before the event enters the MessageBus. Events that don't pass sampling are dropped immediately, reducing unnecessary processing.
How?
guard Sampler(samplingRate: sampleRate).sample() else { return }toTelemetry.metric()andTelemetry.usage()— events are dropped beforesend()is calledstopMethodCalledto callself.metric()instead ofsend(telemetry:)directly, so method call metrics also benefit from call site sampling. RenamedtailSampleRate→callSiteSampleRateto reflect the new semanticsTelemetryReceiver.sampled(event:)and the per-event sampling guards for.metricand.usageinreceive(telemetry:)— the receiver now processes all events that reach itSampledTelemetryconformance fromMetricTelemetryandUsageTelemetrysince the protocol is no longer used for receiver-side samplingsampleRate: 100for data tests,sampleRate: 0for drop tests) and addedtestDefaultSampleRate()tests to pin the concrete default valuesReview checklist
make api-surfacewhen adding new APIs