Add "hints" in Metric API to provide things like histogram bounds #63650
Comments
dotnet-issue-labeler
bot
added
the
untriaged
|
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
tarekgh
added
area-System.Diagnostics.Metric
and removed
untriaged
|
No design has materialized yet, moving this out to Future. |
|
I have a scenario for using this feature. OpenTelemetry has some default bucket bounds for histograms. However, some histograms have recommended bounds. For example,
ASP.NET Core has a very similar histogram, Currently, someone configuring OpenTelemetry to export meter measurements must use the default bucket bounds. To override, you must add a view for that counter to get the desired bucket sizes. services
.AddOpenTelemetry()
.WithMetrics(metrics =>
{
metrics
.AddView("request-duration", new ExplicitBucketHistogramConfiguration
{
Boundaries = new double[] { 0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10 }
})
.AddMeter("Microsoft.AspNetCore.Hosting", "Microsoft.AspNetCore.Server.Kestrel")
.AddPrometheusExporter();
})Histograms should support the |
|
API proposal: namespace System.Diagnostics.Metrics;
+public sealed class HistogramAdvice
+{
+ public IReadOnlyList<double>? ExplicitBucketBoundaries { get; init; }
+}
public class Meter
{
public System.Diagnostics.Metrics.Histogram<T> CreateHistogram<T> (
string name,
string? unit = default,
string? description = default) where T : struct;
+ public System.Diagnostics.Metrics.Histogram<T> CreateHistogram<T> (
+ string name,
+ string? unit = default,
+ string? description = default,
+ HistogramAdvice? advice = default) where T : struct;
}
+public sealed class Histogram<T> : System.Diagnostics.Metrics.Instrument<T> where T : struct
+{
+ public HistogramAdvice Advice { get; }
+}Open Questions
|
|
This is renamed now to "advisory parameters", and there are 2 now - one for histogram custom bounds, one for attributes. |
|
It looks like the explicit histogram advisory bit of the spec is stable so we should be good to move forward with @JamesNK's proposal for .NET 9? Thinking ahead a bit there is also an attributes bit which is experimental. We could add that in the future (if needed) via a base class right? // .NET 10 or whatever
+public class InstrumentAdvice
+{
+ public IEnumerable<KeyValuePair<string, object?>>? Tags { get; set; }
+}
+public sealed class HistogramAdvice : InstrumentAdvice
{
} |
|
What does it mean? The doc says they're recommended. Are they tags that are automatically added to all measurements for the instrument? |
Not exactly. We already have a mechanism for that. The SDK part of the spec has a View API. The View API allows users to manually configure the buckets for explicit bucket histograms as well as select the tags they want from the instrument (among other things). What we are talking about here is the "Advisory API" which is part of the API spec. It sort of mirrors the SDK View API and allows instrumentation to (more or less) pre-configure the default View users will have. They can always use the SDK View API to change the defaults. Let's say we have an Instrument emitting 3 tags: The instrumentation knows one of its tags ("importantTag") most users will care about. It also knows two of its tags ("nonimportantButInterestingToSomeUsersTag1" & "nonimportantButInterestingToSomeUsersTag2") most users won't care about (but some will). Today it emits all three and by default users will get all three. They can use the SDK View API to drop what they don't care about, but that is kind of heavy lifting. Using these attributes on the Advisory API allows the instrumentation to basically pre-configure a default View which will drop the non-important stuff. If users want it, they have to opt-into them using the SDK View API. /cc @alanwest |
public class InstrumentAdvice
{
- public IEnumerable<KeyValuePair<string, object?>>? Tags { get; set; }
+ public IEnumerable<string>? RecommendedTags { get; set; }
} |
|
On the question of whether it should be... public sealed class HistogramAdvice
{
public IReadOnlyList<double>? ExplicitBucketBoundaries { get; init; }
}Or public sealed class HistogramAdvice<T> where T : struct
{
public IReadOnlyList<T>? ExplicitBucketBoundaries { get; init; }
}I did a prototype of this API: namespace System.Diagnostics.Metrics;
+public sealed class HistogramAdvice<T> where T : struct
+{
+ public IReadOnlyList<T>? ExplicitBucketBoundaries { get; init; }
+}
public class Meter
{
public System.Diagnostics.Metrics.Histogram<T> CreateHistogram<T> (
string name,
string? unit = default,
string? description = default) where T : struct;
public Histogram<T> CreateHistogram<T>(
string name,
string? unit,
string? description,
IEnumerable<KeyValuePair<string, object?>>? tags) where T : struct;
+ public System.Diagnostics.Metrics.Histogram<T> CreateHistogram<T> (
+ string name,
+ string? unit,
+ string? description,
+ IEnumerable<KeyValuePair<string, object?>>? tags,
+ HistogramAdvice<T>? advice) where T : struct;
}
+public sealed class Histogram<T> : System.Diagnostics.Metrics.Instrument<T> where T : struct
+{
+ public HistogramAdvice<T>? Advice { get; }
+}Runtime: https://github.com/dotnet/runtime/compare/main...CodeBlanch:metrics-histogramadvice?expand=1 OTel has to do some work to reason out the type of the histogram in order to get at the advice. It isn't much more effort to also convert non-double boundaries to double. So I think we are good to use In OTel if you have a |
|
@CodeBlanch I like your proposal having |
|
@tarekgh The tags part of the spec isn't stable so we don't need it yet. Can you confirm though that if we do something like this in the future (say .NET 10) it would NOT be a breaking change: +public class Advice
+{
+ public IReadOnlyList<string>? RecommendedTags { get; init; }
+}
+public sealed class HistogramAdvice<T> : Advice where T : struct
{
public IReadOnlyList<T>? ExplicitBucketBoundaries { get; init; }
}As long as we can do that in the future I think we're good. |
tarekgh
added
the
api-ready-for-review
|
I have updated the proposal on the top and marked the issue as ready for design review. Please let me know if there is anything missing before we proceed. Thanks all for the feedback! |
namespace System.Diagnostics.Metrics;
public sealed class HistogramAdvice<T> where T : struct
{
public HistogramAdvice(IEnumerable<T> explicitBucketBoundaries);
public IReadOnlyList<T> ExplicitBucketBoundaries { get; }
}
public partial class Meter
{
// NEW: Simple one
public Histogram<T> CreateHistogram<T> (
string name) where T : struct;
// Existing, mark parameters as required and EB never
[EditorBrowsable(EditorBrowsableState.Never)]
public Histogram<T> CreateHistogram<T> (
string name,
string? unit,
string? description) where T : struct;
// Also existing, mark EB never
[EditorBrowsable(EditorBrowsableState.Never)]
public Histogram<T> CreateHistogram<T>(
string name,
string? unit,
string? description,
IEnumerable<KeyValuePair<string, object?>>? tags) where T : struct;
public Histogram<T> CreateHistogram<T> (
string name,
string? unit = default,
string? description = default,
IEnumerable<KeyValuePair<string, object?>>? tags = default,
HistogramAdvice<T>? advice = default) where T : struct;
}
public sealed class Histogram<T> : Instrument<T> where T : struct
{
public HistogramAdvice<T>? Advice { get; }
} |
terrajobst
added
api-approved
|
I think we should make one tweak to the API as proposed: public sealed class HistogramAdvice<T> where T : struct
{
public HistogramAdvice(IEnumerable<T> explicitBucketBoundaries);
- public IReadOnlyList<T> ExplicitBucketBoundaries { get; }
+ public IReadOnlyList<T>? ExplicitBucketBoundaries { get; }
}The reason is, advice may be expanded in the future. There is already stuff in the OTel spec like +public class InstrumentAdvice
+{
+ public Advice(IEnumerable<string>? recommendedTags = default) {}
+ public IReadOnlyList<string>? RecommendedTags { get; }
+}
public sealed class HistogramAdvice<T>
+ : InstrumentAdvice
where T : struct
{
public HistogramAdvice(IEnumerable<T> explicitBucketBoundaries) : this(explicitBucketBoundaries: explicitBucketBoundaries) {}
+ public HistogramAdvice(IEnumerable<string>? recommendedTags = default, IEnumerable<T>? explicitBucketBoundaries = default) : base(recommendedTags) {}
public IReadOnlyList<T>? ExplicitBucketBoundaries { get; }
} |
dotnet-policy-service
bot
added
the
in-pr
|
@CodeBlanch that is fine to have it as |
|
After discussions with @noahfalk, @reyang, @vishweshbankwar, and @tarekgh we decided to make some changes and propose a new API for advice/hints.
Proposalnamespace System.Diagnostics.Metrics;
// NEW
public sealed class InstrumentAdvice<T> where T : struct
{
public IReadOnlyList<T>? HistogramBucketBoundaries { get; init; }
}
public partial class Meter
{
// NEW: Simple one
public Histogram<T> CreateHistogram<T> (
string name) where T : struct;
// Existing, mark parameters as required and EB never
[EditorBrowsable(EditorBrowsableState.Never)]
public Histogram<T> CreateHistogram<T> (
string name,
string? unit,
string? description) where T : struct;
// Also existing, mark EB never
[EditorBrowsable(EditorBrowsableState.Never)]
public Histogram<T> CreateHistogram<T>(
string name,
string? unit,
string? description,
IEnumerable<KeyValuePair<string, object?>>? tags) where T : struct;
// NEW
public Histogram<T> CreateHistogram<T> (
string name,
string? unit = default,
string? description = default,
IEnumerable<KeyValuePair<string, object?>>? tags = default,
InstrumentAdvice<T>? advice = default) where T : struct;
}
public abstract class Instrument<T> : Instrument where T : struct
{
// NEW: Simple one
protected Instrument(Meter meter, string name) {}
// Existing, mark EB never
[EditorBrowsable(EditorBrowsableState.Never)]
protected Instrument(Meter meter, string name, string? unit, string? description) {}
// Also existing, mark EB never
[EditorBrowsable(EditorBrowsableState.Never)]
protected Instrument(Meter meter, string name, string? unit, string? description, IEnumerable<KeyValuePair<string, object?>>? tags) {}
// NEW
protected Instrument(
Meter meter,
string name,
string? unit = default,
string? description = default,
IEnumerable<KeyValuePair<string, object?>>? tags = default,
InstrumentAdvice<T>? advice = default) {}
// NEW
public InstrumentAdvice<T>? Advice { get; }
}
public abstract class Instrument
{
// NEW: Simple one
protected Instrument(Meter meter, string name) {}
// Existing, mark EB never
[EditorBrowsable(EditorBrowsableState.Never)]
protected Instrument(Meter meter, string name, string? unit, string? description) {}
// Existing, decorate optional parameters with defaults
protected Instrument(Meter meter, string name, string? unit = default, string? description = default, IEnumerable<KeyValuePair<string, object?>>? tags = default) {}
} |
|
Is it important to have new simple Instrument constructors? I am guessing the point of these things is to improve the intellisence experience but since I expect user created derivations would be rare it may have low value. No objections to it though if we need it. For HistogramExplicitBucketBoundaries do you think we could shorten the name by removing the word 'Explicit'? It just seemed a little verbose. For InstrumentAdvice if we assume more properties are coming and that folks might want to set somewhat arbitrary subsets of those properties its unlikely we can give each set of properties its own dedicated constructor overload. It also might be odd if someone types |
You don't care about the allocation I guess as instrument creation is not happening much. right? |
|
I just made this change to the proposal in the comment above: public sealed class InstrumentAdvice<T> where T : struct
{
- public InstrumentAdvice(IEnumerable<T> histogramExplicitBucketBoundaries);
- public IReadOnlyList<T>? HistogramExplicitBucketBoundaries { get; }
+ public IReadOnlyList<T>? HistogramBucketBoundaries { get; init; }
}
@noahfalk As far as the ctor changes, I just kind of applied what the reviewers asked for in the first go around. None of it is necessary, just clean up really. I always lean towards consistency personally so I like it but not opposed to dropping it. |
For constructors my suggestion would be leave how it is but just give a heads up to the reviewers that we expect its very rare for 3rd party code to derive their own instrument. As long as that doesn't cause them to change their opinion then consider it resolved :)
Exactly.
Init is one way of doing this, but not the only one. I was trying to avoid init because of the issues that had been raised in the past around it. I was suggesting something like: The copy not only stops people trying to modify the top level property, we could also stop people trying to mutate the nested data structures: It also lets people do this if they want: |
I want to clarify that in the design reviews, it was mentioned before it is ok to use |
tarekgh
added
api-ready-for-review
When you say "with such features", is there going to be guidance in the docs that says what these features are and how users can make use of it in a way that is supported and practical without accidentally using something unsupported? Right now the official doc discourages anyone from trying that strategy and strongly implies that anyone who does this is unsupported and should expect trouble: "Choosing a language version newer than the default can cause hard to diagnose compile-time and runtime errors." |
|
In my last reply, I mentioned |
namespace System.Diagnostics.Metrics;
// NEW
public sealed class InstrumentAdvice<T> where T : struct
{
public IReadOnlyList<T>? HistogramBucketBoundaries { get; init; }
}
public partial class Meter
{
// NEW: Simple one
public Histogram<T> CreateHistogram<T> (
string name) where T : struct;
// Existing, mark parameters as required and EB never
[EditorBrowsable(EditorBrowsableState.Never)]
public Histogram<T> CreateHistogram<T> (
string name,
string? unit,
string? description) where T : struct;
// Also existing, mark EB never
[EditorBrowsable(EditorBrowsableState.Never)]
public Histogram<T> CreateHistogram<T>(
string name,
string? unit,
string? description,
IEnumerable<KeyValuePair<string, object?>>? tags) where T : struct;
// NEW
public Histogram<T> CreateHistogram<T> (
string name,
string? unit = default,
string? description = default,
IEnumerable<KeyValuePair<string, object?>>? tags = default,
InstrumentAdvice<T>? advice = default) where T : struct;
}
public abstract class Instrument<T> : Instrument where T : struct
{
// NEW: Simple one
protected Instrument(Meter meter, string name);
// Existing, mark EB never
[EditorBrowsable(EditorBrowsableState.Never)]
protected Instrument(Meter meter, string name, string? unit, string? description);
// Also existing, mark EB never
[EditorBrowsable(EditorBrowsableState.Never)]
protected Instrument(Meter meter, string name, string? unit, string? description, IEnumerable<KeyValuePair<string, object?>>? tags);
// NEW
protected Instrument(
Meter meter,
string name,
string? unit = default,
string? description = default,
IEnumerable<KeyValuePair<string, object?>>? tags = default,
InstrumentAdvice<T>? advice = default);
// NEW
public InstrumentAdvice<T>? Advice { get; }
}
public abstract class Instrument
{
// NEW: Simple one
protected Instrument(Meter meter, string name);
// Existing, mark EB never
[EditorBrowsable(EditorBrowsableState.Never)]
protected Instrument(Meter meter, string name, string? unit, string? description);
// Existing, decorate optional parameters with defaults
protected Instrument(Meter meter, string name, string? unit = default, string? description = default, IEnumerable<KeyValuePair<string, object?>>? tags = default);
} |
terrajobst
added
api-approved
|
Should |
What do you mean by that? you already can do something like the following: InstrumentAdvice<int> ia = new InstrumentAdvice<int> { HistogramBucketBoundaries = [ 1, 2, 3, 4 ] };Note, in the future, |
Metrics currently supports the Histogram instrument, which is used to publish measurement data. The data aggregator (such as the OpenTelemetry SDK) divides the full interval of possible values into consecutive, non-overlapping ranges called buckets. The OpenTelemetry SDK currently uses some defaults to define the bucket boundaries, but it also provides a way to customize or configure these boundaries. OpenTelemetry has introduced a new specification that allows creators of the Histogram instrument to specify advised bucket boundary values, which will be the recommended values for Histogram data aggregation.
Explicit bucket boundaries OpenTelemetry specs.
Proposal
Usage Example
The text was updated successfully, but these errors were encountered: