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 independent, per-Reader Metric Views configuration #2288

Open
jmacd opened this issue Jan 25, 2022 · 5 comments
Open

Support independent, per-Reader Metric Views configuration #2288

jmacd opened this issue Jan 25, 2022 · 5 comments
Assignees
Labels
spec:metrics Related to the specification/metrics directory

Comments

@jmacd
Copy link
Contributor

jmacd commented Jan 25, 2022

What are you trying to achieve?

Having implemented a mostly-complete Views implementation in open-telemetry/opentelemetry-go#2525, I have the following feature request.

Make it possible to configure Views on a per-Reader basis.

Why? It seems like a shame to specify a powerful way to configure what gets exported from a metrics SDK and not let each Reader configure independent Views. Without a change of the specification, the user who wants to configure independent Views will be required to make a "Multi-MeterProvider" that are configured with one Reader/Views pair each. I view this as a poor outcome because the SDK will lose any opportunity to share work among the two readers, particularly for synchronous instruments.

I make this request after implementing the draft PR linked above, where it looks like only a slight change. The way that PR is structured, there's almost no difference between supporting independent views because an "intermediate" aggregation is computed for synchronous instruments. No matter how many Views are attached to the same synchronous instrument, only distinct aggregations need to be computed on the fly. It means that if you configure 1 Counter instrument and 2 different Views over different attribute sets (with different output names, of course), the runtime would track 1 intermediate aggregation (having Delta temporality) and then output into each View on collection. The SDK just knows that it has a number of destinations per intermediate aggregation; it doesn't matter how many readers there are, at least the way I've structured it there.

Example

The current specification has an example like this:

# metrics from X and Y (reported as Foo and Bar) will be exported
meter_provider
    .add_view("X")
    .add_view("Foo", instrument_name="Y")
    .add_view(
        "Bar",
        instrument_name="Y",
        aggregation=HistogramAggregation(buckets=[5.0, 10.0, 25.0, 50.0, 100.0]))
    .add_metric_reader(PeriodicExportingMetricReader(ConsoleExporter()))

With the proposed change, we could write examples like this:

# Instrument Foo is renamed differently for the two exporters 
meter_provider
    .add_metric_reader(PullExportingMetricReader(PrometheusExporter())
        .add_view("FooRenamedTheOtherWay", instrument_name="Foo")),
    .add_metric_reader(PeriodicExportingMetricReader(OTLPExporter())
        .add_view("FooRenamedOneWay", instrument_name="Foo"))

We could configure ExponentialHistograms for OTLP while configuring ExplicitBucketHistograms for Prometheus, for example, or limit the set of attributes differently for these two exporters.

@jmacd jmacd added the spec:metrics Related to the specification/metrics directory label Jan 25, 2022
@jmacd jmacd added this to the Metrics API/SDK Stable Release milestone Jan 25, 2022
@jsuereth
Copy link
Contributor

I'm curious what you think about the cost of maintaining two different histogram implementations inside the SDK.

I love the idea behind this, but I'm a bit worried about the overhead.

@jmacd
Copy link
Contributor Author

jmacd commented Feb 4, 2022

I expected we would maintain two histogram implementations because we have two point kinds, and neither can exactly implement the other. However, I'd be glad to let explicit-boundary histograms be generated (somewhat imprecisely) by exponential histograms.

The same arguments would apply to other aggregations though. For OTLP push, I want to push full cardinality, whereas for Prometheus pull I want to export limited cardinality. It would be the same aggregators with different configurations.

@MadVikingGod
Copy link
Contributor

MadVikingGod commented Sep 29, 2022

Should we have some clarification of what should happen to views that are registered outside of the scope of a MetricReader?
For example, what should I expect if I do something like:

# What Metrics do I get from readers foo, and bar?
meter_provider
    .add_metric_reader(foo)
meter_provider
    .add_view("X")

 meter_provider
    .add_metric_reader(bar) 

What about:

# What Metrics do I get from reader foo?
meter_provider
    .add_view("*", aggregation=Drop)
    .add_metric_reader(foo) 
meter_provider
    .add_view("X")

Are the views attached to one reader applied to others?

# What Metrics do I get from readers foo and bar?
meter_provider
    .add_view("Foo", instrument_name="Y")
    .add_metric_reader(foo) 
meter_provider
    .add_view("Foo", instrument_name="X")
    .add_metric_reader(bar) 

@MrAlias
Copy link
Contributor

MrAlias commented Oct 19, 2022

If this is accepted, the existing approach of configuring view(s) with the MeterProvider will need to be preserved. Stable implementations of the specification have already been released with this language.

Registering views with readers will need to be considered as an extension. The remaining question is if new language implementations will need to be required to also make their implementations accept view(s) to be registered with the MeterProviders?

@ahayworth
Copy link
Contributor

I feel there could be utility in per-reader views, personally. For example, suppose I had two metric readers - one paired with an OTLP push exporter, and the other with a Prometheus exporter. I might, potentially, wish to drop certain metrics or aggregate across certain labels for one of those exporters - but not the other. It would be difficult to implement this today without per-reader views.

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
Projects
None yet
Development

No branches or pull requests

7 participants