-
Notifications
You must be signed in to change notification settings - Fork 956
Add OptIn metric advisory parameter #4809
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
base: main
Are you sure you want to change the base?
Conversation
5e2339b to
b5508fd
Compare
8f015e6 to
a06fc8d
Compare
| [Drop Aggregation](#drop-aggregation) by default. If the user has provided an | ||
| Aggregation via View(s), that aggregation takes precedence. | ||
|
|
||
| ### Instrument enabled |
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.
Should this (###Instrument Enabled) section be updated to return false based on DefaultDisabled?
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.
I initially included that, but I think it is covered by the "resolved view" clause:
opentelemetry-specification/specification/metrics/sdk.md
Lines 1009 to 1010 in d783245
| - All [resolved views](#measurement-processing) for the instrument are | |
| configured with the [Drop Aggregation](#drop-aggregation). |
The #measurement-processing section linked there is what I modify here: https://github.com/open-telemetry/opentelemetry-specification/pull/4809/files#diff-32d5f289beed96900fa1e6e69a3ee9cc0b6503ce8883672011c44bc772c0fa2aR424.
So my reading of it is that updating the #measurement-processing section is enough to make Instrument Enabled return false based on DefaultDisabled.
cijothomas
left a comment
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.
Agree with the change, except the name. I prefer OptIn.
Requesting changes only for the name part.
a06fc8d to
c3548c2
Compare
|
@cijothomas Name was changed to |
|
Feedback from 2/6 spec sig meeting:
|
4ed0da2 to
cedcd06
Compare
|
I've given metric levels more thought, and have looked through how the collector has implemented them. I updated the alternatives considered section in the description. TL;DR: Metric Levels should be layered on-top-of views, and should not be part of the API. |
codeboten
left a comment
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.
These are all tuned today using views. Instead of building metric levels into the API, we should build metric levels as a layer on top of views. That is actually what the collector does today.
It's not immediately clear to me how levels could be layered on top of views, as instrumentation authors need to be able to define the level of a metric in their definition of the instrument, and views are configured when instantiating the provider.
The collector can do this as it is an end user of the SDK, and chooses what telemetry to emit at various levels via views, but as an instrumentation author i'm not sure how that would work w/ the api we have available today.
Would a "level" metric advisory parameter work here?
5f77f6d to
f9f94ef
Compare
I'm hoping to avoid a level advisory parameter. See #4828 for a rough sketch of what I had in mind. |
|
@MrAlias @reyang (since you have already approved) please see recent change based on this discussion: #4809 (comment) |
|
This was discussed at the 1/13 Spec SIG meeting today:
|
scope level might work for them. I don't think a global setting would though. |
jack-berg
left a comment
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.
I'm partial to an approach where these instruments are enabled via the MeterConfig / MeterConfigurator mechanism. As discussed in the spec SIG a couple of times, this issue of config knob granularity (i.e. should the knob be at the meter provider, metric reader, meter, or instrument level) is recurring, and I think the answer ends up being subjective rather than right or wrong. Reason I prefer config at the meter level:
- The view API has surprising behavior when conflicts occur. The idea of a using a wildcard view to opt into all opt-in metrics only works if you currently don't have any other views. As soon as you do, that wildcard view will yield duplicate metric streams for all "enabled by default metrics". Because of this, I generally try to stay away from views. When I do use / recommend them, I recommend selecting / configuring individual instruments, which avoids the no-merging / duplicate stream problems.
- Adding a new "enabled" field to views seems like the most correct thing to do here given the discussion, and yet does jive well with the existing view properties. Had we had this "enabled" concept from v1, we wouldn't need the "drop" aggregation. But we do have "drop" and with "enabled" there are now two ways to do the same thing.
- The meter level seems sufficiently granular. Views are of course more granular, but I would guess that meter level configuration seems to be good enough for most usage.
- If the meter config option manifested as something like a "minimum requirement level", then it would have symmetry to the
minimum_severityproperty of LoggerConfig. Its reasonable to see how that idea could be extended to traces such that we could have a threshold / level property in LoggerConfig / MeterConfig / TracerConfig.
That's my preference, but I understand the desire / argument for configuration at the view level and so won't block.
|
@cijothomas @reyang @MrAlias please re-review -- especially the new view Stream.Enabled field. Also let me know if you would prefer that I explore a meter-level configuration for enabling opt-in metrics instead. |
|
I updated the PoC to use the new Enabled field on the View's stream. |
e8a0f94 to
86a8835
Compare
Co-authored-by: Tyler Yahn <[email protected]>
Co-authored-by: Cijo Thomas <[email protected]>
86a8835 to
4704d7e
Compare
|
One thing I found when implementing the prototype is that it is important for |
|
I've also added hypothetical declarative configuration examples in the description. |
Fixes #4391
Changes
Add a new advisory parameter, OptIn, which causes the SDK to use the DropAggregation by default. The instrument's Enabled method returns
falseby default.Users can enable metrics by using Views. They can set a new
enabledparameter to true in the stream configuration to do this. They can enable all metrics by setting enabled=true with a wildcard (*) view.Example in Declarative Config
If accepted, this would likely be an enabled: field within stream configuraiton.
A user could enable a single instrument with a view:
A user could disable a single instrument with a view:
A user could enable all OptIn instruments:
Motivation
I would like to be able to metric semantic conventions with the opt-in requirement level. In particular, I am interested in this to be able to define some for Go runtime semantic conventions. See open-telemetry/opentelemetry-go-contrib#6321 (comment).
Prototypes
Alternative Considered: DefaultAggregation enables metrics
Instead of a new
enabledparameter in View, we could just tell users to useaggregation=DefaultAggregationto enable OptIn metrics.The issue is that DefaultAggregation is often used in Views to make a small change (enable exponential histograms, change default histogram boundaries, etc), but otherwise leave the aggregation unmodified. If we use DefaultAggregation as a signal to enable OptIn metrics, it prevents DefaultAggregation from being used as a signal to "keep the normal aggregation behavior".
Alternative Considered: Metric Levels
TL;DR: Metric Levels should be built on-top-of views, and can co-exist with this OptIn parameter.
In a similar vein to #3205, we could define levels for metrics. As an instrumentation author, instead of defining a metric as "default disabled", I would define a metric as "debug level". Users would be able to enable all debug metrics as a group, similar to how debug logging is turned on today.
A complete metric levels feature should control more than just which instruments are enabled or disabled. The overall goal should be managing cardinality and data size, which has many other dimensions. I've opened #4828 with details about how levels could be layered on-top-of views, as an alternative to adding them to the API.
CHANGELOG.mdfile updated for non-trivial changes