Skip to content
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

Support disabled-by-default metrics using an advisory parameter. #4391

Open
dashpole opened this issue Jan 30, 2025 · 9 comments
Open

Support disabled-by-default metrics using an advisory parameter. #4391

dashpole opened this issue Jan 30, 2025 · 9 comments
Labels
spec:metrics Related to the specification/metrics directory triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted

Comments

@dashpole
Copy link
Contributor

What are you trying to achieve?

As an instrumentation author, I would like to record metrics that are disabled by default. Today, I need to add configuration to my instrumentation library to support enabling or disabling metrics.

As an end-user, I would like to be able to use a View (or other similar mechanism) to enable default-disabled metrics--ideally in a way that is compatible with the file-based SDK configuration so I can enable such metrics at runtime.

Proposed solution

I propose adding a new advisory parameter to the metrics Instrument API: "DefaultDisabled" (naming depends on the outcome of #4344). If this parameter is provided on instrument creation, the SDK will use the DropAggregation by default instead of the standard default.

A user could enable this metric by changing the Aggregation of the metric stream from DropAggregation to the DefaultAggregation.

This is very similar to the approach taken by the Attributes advisory parameter, in that it allows instrumentation authors to provide verbose telemetry using the instrument API, but specify a different default for how it is aggregated on instrument creation.

Alternatives considered

  • We could take a more general approach, and support a "DefaultAggregation" advisory parameter. This would allow something like measuring using histogram instrument, but defaulting to aggregating as a gauge. I'm not convinced those kinds of use-cases exist, though.

Additional context.

This came up in the context of open-telemetry/opentelemetry-go-contrib#6321 (comment).

@dashpole dashpole added the spec:metrics Related to the specification/metrics directory label Jan 30, 2025
@dashpole
Copy link
Contributor Author

cc @MrAlias

@cijothomas
Copy link
Member

cijothomas commented Jan 31, 2025

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#meterconfigurator
Could the instrumentation's Meter (Scope) name be used to disable it by the end user? (No need of Views)

(Agree this is not something the instrumentation author can control.)

@trask
Copy link
Member

trask commented Jan 31, 2025

I really like the idea of having a standard way to implement disabled-by-default metrics.

We have several of these in Java instrumentation, and each one currently needs to have its own config property for users to be able to enable it.

@mx-psi
Copy link
Member

mx-psi commented Feb 10, 2025

This would be really helpful for the Collector as well, where we want to have some mechanism for components to report what metrics should be disabled with the default level (see open-telemetry/opentelemetry-collector/issues/11754)

@danielgblanco danielgblanco added the triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted label Feb 10, 2025
@danielgblanco
Copy link
Contributor

We (@svrnm and @mx-psi in triage meeting) think this is a good idea. @MrAlias, as spec sponsor would you be willing to sponsor this?

@jade-guiton-dd
Copy link

jade-guiton-dd commented Feb 10, 2025

What is the plan on how DefaultDisabled would interact with MeterConfig and MeterConfigurator? Would MeterConfig.disabled be set to DefaultDisabled if the MeterConfigurator returns "some signal indicating that the default MeterConfig should be used"?

@dashpole
Copy link
Contributor Author

@jade-guiton-dd this issue is about providing instrument-level configuration similar to other advisory parameters. If the Meter is disabled, no metrics will be collected, regardless of this setting. If the meter is enabled, only then would this (and other instrument-level) configuration apply.

@lmolkova
Copy link
Contributor

lmolkova commented Feb 11, 2025

I really like this idea.

I wish we could have a thing that supports more than 2 states so it could evolve. Using logging severity as analogy, I wish we had a config like

enum Importance: 
   High,
   Medium
   Low,

or

enum DefaultStatus # needs a better name
   Enabled, 
   Disabled,
   # we could add things in the future, but it'd still be limiting

We haven't even started to figure it out - #3205, so I think we'd have to start with a boolean flag anyway.


A separate note: there were discussions about double negation recently #4344 #4351 and I think it'd be better to avoid using it here. I.e. instead of DefaultDisabled I'm suggesting DefaultStatus = Enabled or DefaultEnabled = true

@dashpole
Copy link
Contributor Author

@lmolkova Interesting idea. I wonder if that should be a higher-level configuration concept, since we already have knobs for selecting which attributes are disabled by default. I would imagine that getting "detailed" metrics would enable both default-disabled metrics, and default-disabled attributes. Maybe it should even change the aggregation for some metrics (e.g. it is a counter when getting "basic" metrics, but a histogram when getting "detailed" metrics)?

I do worry that giving instrumentation authors a log-level-like knob will lead to the same inconsistencies we see with logging, where everyone has a different definition of "Info", and libraries use it inconsistently, which requires us to add additional config knobs to allow users to fix. If there was some way for us to objectively define levels for metrics (e.g. "detailed" means enable all metrics, and "debug" means enable all metrics and all attributes) which could be layered on top of SDK config, that seems better to me than handing instrumentation authors a knob similar to log-level.

So I think the simpler option i'm proposing above is still the right first step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:metrics Related to the specification/metrics directory triage:deciding:community-feedback Open to community discussion. If the community can provide sufficient reasoning, it may be accepted
Projects
None yet
Development

No branches or pull requests

7 participants