feat(metrics): Trace-connected Metrics (Implementation) #4834
Conversation
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4834 +/- ##
==========================================
- Coverage 73.84% 73.76% -0.09%
==========================================
Files 483 495 +12
Lines 17577 17893 +316
Branches 3464 3512 +48
==========================================
+ Hits 12979 13198 +219
- Misses 3742 3832 +90
- Partials 856 863 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
src/Sentry/SentryMetric.cs
Outdated
| public void SetAttribute(string key, object value) | ||
| { | ||
| _attributes ??= new Dictionary<string, object>(); | ||
|
|
||
| _attributes[key] = new SentryAttribute(value); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…rics Upgrades Sentry .NET SDK from 6.0.0 to 6.1.0-alpha.1 to test the new experimental trace-connected metrics API from PR getsentry/sentry-dotnet#4834. Changes: - Upgrade Sentry SDK to 6.1.0-alpha.1 - Add SentryClientMetrics decorator that emits metrics to Sentry - Enable experimental metrics in all apps (Server, Console, Android) - Use SentryClientMetrics in DI registrations The new metrics API provides: - AddCounter: for counting events (files, uploads, errors) - RecordDistribution: for value distributions (uploaded bytes) - RecordGauge: for point-in-time values (jobs in flight) Metrics are automatically correlated with the active trace/span. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…rics Upgrades Sentry .NET SDK from 6.0.0 to 6.1.0-alpha.1 to test the new experimental trace-connected metrics API from PR getsentry/sentry-dotnet#4834. Changes: - Upgrade Sentry SDK to 6.1.0-alpha.1 - Add SentryClientMetrics decorator that emits metrics to Sentry - Enable experimental metrics in all apps (Server, Console, Android) - Use SentryClientMetrics in DI registrations (Core, Server, Console) - Fix duplicate ClientMetrics registration in Core/Startup.cs The new metrics API provides: - AddCounter: for counting events (files, uploads, errors) - RecordDistribution: for value distributions (uploaded bytes) - RecordGauge: for point-in-time values (jobs in flight) Metrics are automatically correlated with the active trace/span. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
…rics Upgrades Sentry .NET SDK from 6.0.0 to 6.1.0-alpha.1 to test the new experimental trace-connected metrics API from PR getsentry/sentry-dotnet#4834. Changes: - Upgrade Sentry SDK to 6.1.0-alpha.1 - Add SentryClientMetrics decorator that emits metrics to Sentry - Make ClientMetrics methods virtual for proper polymorphism - Enable experimental metrics in all apps (Server, Console, Android) - Use SentryClientMetrics in DI registrations (Core, Server, Console) - Fix duplicate ClientMetrics registration in Core/Startup.cs The new metrics API provides: - AddCounter: for counting events (files, uploads, errors) - RecordDistribution: for value distributions (uploaded bytes) - RecordGauge: for point-in-time values (jobs in flight) Metrics are automatically correlated with the active trace/span. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
#252) * feat: upgrade Sentry SDK to 6.1.0-alpha.1 and add trace-connected metrics Upgrades Sentry .NET SDK from 6.0.0 to 6.1.0-alpha.1 to test the new experimental trace-connected metrics API from PR getsentry/sentry-dotnet#4834. Changes: - Upgrade Sentry SDK to 6.1.0-alpha.1 - Add SentryClientMetrics decorator that emits metrics to Sentry - Make ClientMetrics methods virtual for proper polymorphism - Enable experimental metrics in all apps (Server, Console, Android) - Use SentryClientMetrics in DI registrations (Core, Server, Console) - Fix duplicate ClientMetrics registration in Core/Startup.cs The new metrics API provides: - AddCounter: for counting events (files, uploads, errors) - RecordDistribution: for value distributions (uploaded bytes) - RecordGauge: for point-in-time values (jobs in flight) Metrics are automatically correlated with the active trace/span. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * test: add unit tests for SentryClientMetrics Adds tests to verify that: - SentryClientMetrics emits trace_metric items to Sentry - All metric types (Counter, Distribution, Gauge) are emitted - Base class counters are also incremented (dual emission) - Polymorphism works correctly (override vs new) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * refactor: use IHub for SentryClientMetrics to improve testability - SentryClientMetrics now accepts IHub in constructor (defaults to HubAdapter.Instance) - Tests use isolated SDK instances with recording transport - Each test initializes/disposes its own SDK for proper isolation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> * Delete test/SymbolCollector.Core.Tests/SentryClientMetricsTests.cs --------- Co-authored-by: Claude Opus 4.5 <[email protected]>
Provides hierarchical constants for metric units supported by Sentry Relay, organized into Duration, Information, and Fraction categories. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
| /// <param name="value">The value of the metric.</param> | ||
| /// <typeparam name="T">The numeric type of the metric.</typeparam> | ||
| /// <remarks>Supported numeric value types for <typeparamref name="T"/> are <see langword="byte"/>, <see langword="short"/>, <see langword="int"/>, <see langword="long"/>, <see langword="float"/>, and <see langword="double"/>.</remarks> | ||
| public void EmitCounter<T>(string name, T value) where T : struct |
There was a problem hiding this comment.
question: discuss identifiers of public method groups
See spec in dev docs Metrics > Metrics Module
-Sentry.metrics.count(..)
+SentrySdk.Metrics.EmitCounter(..);
-Sentry.metrics.gauge(..)
+SentrySdk.Metrics.EmitGauge(..);
-Sentry.metrics.distribution(..)
+SentrySdk.Metrics.EmitDistribution(..);countis both a verb and a noungaugeis both a verb and a noundistributionis a noun only
So my thought was to - in particular for Distribution - use a verb as a prefix to form a verb phrase.
And then - for consistency - use a verb as a prefix for all three method groups.
The respective .NET Metrics (and Open Telemetry) are
using System.Diagnostics.Metrics;
static Meter meter = new Meter(name: "my-meter");
static Counter<int> counter = meter.CreateCounter<int>(name: "my-counter");
static Gauge<int> gauge = meter.CreateGauge<int>(name: "my-gauge");
static Histogram<int> histogram = meter.CreateHistogram<int>(name: "my-histogram");
counter.Add(delta: 1);
gauge.Record(value: 2);
histogram.Record(value: 3);The first draft started with
SentrySdk.Metrics.AddCounter
SentrySdk.Metrics.RecordGauge
SentrySdk.Metrics.RecordDistributionalthough with the verbs consistent with .NET/Open-Telemtry, these verbs do not fit, as they may suggest client-side aggregation, rather than sending the metric as-provided to Sentry with query-time aggregation at Sentry.
Then changing to the verb Emit. This verb is also used in the dev docs. The verb Emit is also used in Logs in Sentry for Go.
So the current naming scheme is, almost, Emit{Type}.
However, currently, instead of EmitCount, I decided to use EmitCounter instead, to be closer to the .NET BLC / System.Diagnostics.DiagnosticSource NuGet package.
An argument can be made to instead be consistent with the spec ... or at least more consistent with the spec. So here are some alternatives up for discussion.
// "type": "counter"
SentrySdk.Metrics.Count() // as in spec: "Sentry.metrics.count()"
SentrySdk.Metrics.EmitCount() // as verb phrase
SentrySdk.Metrics.EmitCounter() // Counter instead of Count, for consistency with the actual type name
// "type": "gauge"
SentrySdk.Metrics.Gauge() // as in spec: "Sentry.metrics.gauge()"
SentrySdk.Metrics.EmitGauge() // as verb phrase
// "type": "distribution"
SentrySdk.Metrics.Distribution() // as in spec: "Sentry.metrics.distribution()"
SentrySdk.Metrics.EmitDistribution() // as verb phraseWhat do you prefer?
UPDATE: oh no ... I just realized we had old metrics before ... should we restore (or adapt parts of) that API shape?
There was a problem hiding this comment.
I don't think we have to worry about the API shape of the previous metrics product we had. I think the naming as you have it is fine.
| /// <param name="value">The value of the metric.</param> | ||
| /// <typeparam name="T">The numeric type of the metric.</typeparam> | ||
| /// <remarks>Supported numeric value types for <typeparamref name="T"/> are <see langword="byte"/>, <see langword="short"/>, <see langword="int"/>, <see langword="long"/>, <see langword="float"/>, and <see langword="double"/>.</remarks> | ||
| public void EmitGauge<T>(string name, T value) where T : struct |
There was a problem hiding this comment.
question: Overloads and Parameters of Method Groups
The SDK spec Metrics > Metrics Module suggests these parameters for the three methods Sentry.metrics.count, Sentry.metrics.gauge, and Sentry.metrics.distribution:
name, String, requiredvalue, Number, requiredoptions, Object, optionalunit, String, optional, only for distribution and gauge, should not be anenumattributes, Object, optionalscope, Scope, optional
name
I decided to have several overloads per method group, rather than a single method per type. This is also what the Meter type / Instrument types of the net6.0+ BCL APIs do.
value
With .NET being typed, I decided to make the APIs generic, as this is also what the .NET BCL / Open Telemetry / System.Diagnostics.DiagnosticSource NuGet package does.
This supports the numeric types byte, short, int, long, float, double. But don't support decimal (.NET Instruments do support it) as it's 128-bit, and Sentry only supports 64-bit signed integral types and 64-bit floating-point numbers.
options
Instead of an Options-Bag, I went with a method group and respective overloads, as again, they are similar to the System.Diagnostics.Metrics APIs.
However, if we'd like, we could do a struct-based Options-bag, per type (struct CounterOptions, struct GaugeOptions, and struct DistributionOptions).
unit
The unit overload only exists in the Gauge and Distribution method groups.
Typed to string, rather than Sentry.MeasurementUnit, as the SDK spec suggests to:
SDKs must not restrict what can be sent in (e.g. by using an enum) but should offer constants or similar that help customers send in units we support.
We could, however, offer an overload that takes Sentry.MeasurementUnit instead of a string.
Or we could add this overload later on.
The motivation of having a string rather than an enum is that Sentry may start supporting more Units at some time, and SDK users should be able to pass them once supported without upgrading the SDK.
Currently, a "custom" unit is not supported, and Relay will either discard or set the Unit to None if an unsupported unit is passed by the user --- the metric itself will not be discarded in that case.
attributes
Offer both an IEnumerable<KeyValuePair<string, object>>, and a ReadOnlySpan<KeyValuePair<string, object>> overload, again, very similar to "tags" of System.Diagnostics.Metrics.
scope
Just a nullable Scope.
Currently not available, but we could now - or later - add an overload for passing a Scope without a unit:
..(string name, T value, Scope? scope)
What do you think?
UPDATE: oh no ... I just realized we had old metrics before ... should we restore (or adapt parts of) that API shape?
There was a problem hiding this comment.
Instead of an Options-Bag, I went with a method group and respective overloads, as again, they are similar to the System.Diagnostics.Metrics APIs.
Using an Options type would mean less overloads to maintain... and if the options type was sealed then we could add members to it later. It's probably easier and less ceremony for SDK users to use various overloads with the options they care about.
It really depends how many dimensions the options type exposes... eventually (like with the SentryOptions) the only practical way to set these is via some kind of callback/delegate.
I think it's fine as is for now.
There was a problem hiding this comment.
Currently not available, but we could now - or later - add an overload for passing a Scope without a unit:
..(string name, T value, Scope? scope)
What was the rational for omitting this overload currently?
There was a problem hiding this comment.
Scope currently does not have Attributes yet: opened #4882.
Also the Scope type isn't too prominent yet::
SentrySdk.ConfigureScopedoesn't return a scope .. but the user could "capture" it out of the delegateSentrySdk.PushScopereturns anIDisposable(instance of internalScopeSnapshot)
I was thinking of adding it when we've added Attributes to Scope in the follow-up.
But we could add and approve it right away to get feedback on the full set of overloads during the experimental phase.
src/Sentry/SentryOptions.cs
Outdated
| /// Supported numeric value types for <typeparamref name="T"/> are <see langword="byte"/>, <see langword="short"/>, <see langword="int"/>, <see langword="long"/>, <see langword="float"/>, and <see langword="double"/>. | ||
| /// </remarks> | ||
| /// <seealso href="https://develop.sentry.dev/sdk/telemetry/metrics/"/> | ||
| public void SetBeforeSendMetric<T>(Func<SentryMetric<T>, SentryMetric<T>?> beforeSendMetric) where T : struct |
There was a problem hiding this comment.
question: Design
With the Metric type being generic, supporting byte, short, int, long, float, and double, this API is also quite similar to the MeterListener of System.Diagnostics.Metrics.
Where you can set one callback per metric type via SetMeasurementEventCallback.
See Collect metrics.
What do you think?
UPDATE: oh no ... I just realized we had old metrics before ... should we restore (or adapt parts of) that API shape?
There was a problem hiding this comment.
It's a pain since the SDK user may want a callback like:
if (metric.attributes.contains(a => a.name == "foo"))
{
return null;
}
But they'd need to define that function once for each numeric type our metrics support and if we add support for new numeric types their code might break. It doesn't feel like a great API design.
An alternative option (not saying it's better, but just to consider) would be to have the parameter to the callback be a flattening of the various generic types... e.g.
SentryMetricArgType
{
SentryMetricType type;
string name;
object value;
string? unit;
IEnumerable<KeyValuePair<string, object>> attributes;
Scope? scope;
}
That might be a bit messy: impedance mismatch, mappings, type safety issues etc. but it might be better than the alternative.
A more complex variant would be to pass something like a SentryMetricTypeAccessor that mitigated some of the type safety issues by exposing properties of the metric that the SDK user might want to read via getter methods and exposed properties the SDK user might want to modify (if any) via setter methods... so they could do something like:
options.Metrics.SetBeforeSend(accessor => {
if (accessor.GetMetricValue() is double && accessor.Attributes.Contains("foo"))
{
return null;
}
});
Probably have to play around with different options to see how the implementation and SDK usage feels in practice. I could see what we have in this PR at the moment being pretty onerous for SDK users though, so I think we need to find a better option.
What solution have the other strongly typed SDKs (like Java) opted for here?
There was a problem hiding this comment.
re: Attributes
I'd like to "fix" them during a separate follow-up PR, if you don't mind,
to get a dedicated design review on this API / these APIs,
as it will impact not only Metrics, but Logs and Scopes (for now) as well.
Opened #4882.
For now, I went with similar SetAttribute/TryGetAttribute methods as we have for Logs.
We haven't "burned" the public identifier Attributes on Logs yet, so we can then adapt the same Collection-based property for Logs as well, after shipping and testing it during the experimental phase for Metrics.
There was a problem hiding this comment.
re: generic SetBeforeSendMetric Options-API
You are right ... if we'd add a new Value-Type (Relay/Sentry may support 128-bit integral/floating-point numbers later on), this would be a breaking change indeed.
Other typed SDKs just have a double.
sentry-cocoa, having Metrics in Swift, uses UInt for count and Double for distribution+gauge.
I started out with a double as well, but ran into a loss-of-precision problem during testing when converting from float:
Metrics.EmitCounter<float>("", 1.1f); // { "value": 1.1 }
Metrics.EmitCounter<double>("", 1.1d); // { "value": 1.1 }
Metrics.EmitCounter<double>("", 1.1f); // { "value": 1.100000023841858 }Well ... we could argue
- the potential imprecision when floats should be used are neglectable
- Relay/Sentry probably won't support > 64-bit numbers anytime soon ... maybe ever
So maybe just a double is perfectly fine ... similar to the "old" Metrics 🤔
| /// <remarks> | ||
| /// Experimental features are subject to binary, source and behavioral breaking changes in future updates. | ||
| /// </remarks> | ||
| public ExperimentalSentryOptions Experimental { get; } |
There was a problem hiding this comment.
question: Experimental vs Stable.
Do we want to publish it as experimental first ... or stable directly?
There was a problem hiding this comment.
There's quite a lot of code so we could publish it as experimental for one point release... try to get some feedback from SDK customers using it in anger. There may be strong opinions some of the design decisions (e.g. method groups vs. options types/callbacks) so good to have the flexibility to revisit some of those decisions for a wee while if need be.
Otherwise it's more of a commercial question (is Sentry sure we're going to GA this metrics product?). I assume we captured enough learnings from the first metrics product to be pretty confident of the design and cost structure this time around. @dingsdax ?
There was a problem hiding this comment.
We'll release it as Experimental feature initially, but going stable by March.
|
|
||
| namespace Sentry.Testing; | ||
|
|
||
| public sealed class InMemorySentryTraceMetrics : SentryTraceMetrics |
There was a problem hiding this comment.
Where does this get used?
There was a problem hiding this comment.
Let's merge them now... the code is tightly coupled anyway - I think it's harder to review having them in separate PRs.
Co-authored-by: James Crosswell <[email protected]>
| /// Defaults to <see langword="true"/>. | ||
| /// </summary> | ||
| /// <seealso href="https://develop.sentry.dev/sdk/telemetry/metrics/"/> | ||
| public bool EnableMetrics { get; set; } = true; |
There was a problem hiding this comment.
Experimental metrics enabled by default, unlike logs
Low Severity
The EnableMetrics property defaults to true, while the similar EnableLogs property defaults to false. Both are telemetry features that create BatchProcessor infrastructure, but they have inconsistent default behaviors. This means SDK users upgrading will automatically have metrics infrastructure created even though it's marked as experimental, whereas logs require explicit opt-in. Experimental features typically default to disabled to require deliberate opt-in.
There was a problem hiding this comment.
That's a fair comment... enabling by default will result in build warnings or failures for folks upgrading to the new version. If it's experimental, we should disable by default.
There was a problem hiding this comment.
It was discussed whether the default for Metrics be true or false.
The decision was made to be true per default. In particular because we don't have a default integration (currently) and the user needs to actively use the Metrics-APIs in order to use Sentry Metrics.
Moreover the boolean flag has no influence on any experimental build warnings, as the SDK-APIs need to be used actively. I'd like to keep the default here to true.
https://develop.sentry.dev/sdk/telemetry/metrics/#initialization-options
|
|
||
| Debug.Assert(hub is not Hub, "In case of a 'full' Hub, there is always a Scope. Otherwise (disabled) there is no Scope, but this branch should be unreachable."); | ||
| traceId = SentryId.Empty; | ||
| } |
There was a problem hiding this comment.
Duplicate GetTraceIdAndSpanId method between logs and metrics
Low Severity
The GetTraceIdAndSpanId method in SentryMetric.Factory.cs is nearly identical to the one in SentryLog.cs. Both methods retrieve trace and span IDs from an active span or scope's propagation context, with only a minor comment difference. This duplicated logic increases maintenance burden and risks inconsistent bug fixes if changes are made to one but not the other.
| { | ||
| SetAttribute("sentry.sdk.version", version); | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate SetDefaultAttributes method between logs and metrics
Low Severity
The SetDefaultAttributes method in SentryMetric.cs is identical to the one in SentryLog.cs. Both set the same four attributes (sentry.environment, sentry.release, sentry.sdk.name, sentry.sdk.version) with identical logic. This duplication increases maintenance burden and risks inconsistent implementation.
|
|
||
| internal void Apply(Scope? scope) | ||
| { | ||
| } |
There was a problem hiding this comment.
Empty Apply method is called but does nothing
Low Severity
The Apply(Scope? scope) method in SentryMetric has an empty body but is actively called from SentryMetric.Create in the factory. This is dead code that adds unnecessary complexity. Unlike SentryLog, which doesn't have an Apply method at all, this suggests incomplete implementation or premature scaffolding.
Additional Locations (1)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
|
|
||
| internal ExperimentalSentryOptions(SentryOptions options) | ||
| { | ||
| _options = options; |
There was a problem hiding this comment.
Unused _options field in ExperimentalSentryOptions
Low Severity
The _options field in ExperimentalSentryOptions is declared and assigned in the constructor but never read or used anywhere in the class. This is dead code that clutters the codebase and may confuse future developers about its intended purpose.
Implementation of Trace-connected Metrics
See https://develop.sentry.dev/sdk/telemetry/metrics/
This changeset contains the initial implementation of Trace-connected Metrics for Sentry.
Changes
SentryTraceMetricsand derived), which follow the same pattern and structure from Structured LogsSystem.Diagnostics.DiagnosticSourcepackagePlease see the discussion comments below to discuss these design decisions.
Related changesets
Related issues and discussions
Follow-ups
Enable public beta
dotnetplatform