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

A way to specify metric aggregator #1465

Closed
morigs opened this issue Aug 26, 2020 · 11 comments
Closed

A way to specify metric aggregator #1465

morigs opened this issue Aug 26, 2020 · 11 comments
Labels
question User is asking a question not related to a new feature or bug

Comments

@morigs
Copy link
Contributor

morigs commented Aug 26, 2020

I cannot believe this functionality is still missing but also I cannot find any related issues/PRs. Feel free to close if it's a duplicate.

I am a library author. I need to expose a "latency" metric with ability to discover its avg and max values in the future. According to the otel spec I have chosen ValueRecorder:

const latencyRecorder = meter.createValueRecorder('processing_latency', { description: 'Processing latency.' });

As we can see here the default aggregator for ValueRecorder is MinMaxLastSumCountAggregator. It seems MinMaxLastSumCountAggregator is suits me. Unfortunately MinMaxLastSumCountAggregator metric is exported as a single value...
I want to change aggregator to histogram (and use Prometheus histogram functions for analytics).
Unfortunately there is no way to specify aggregator during metric creation. Batcher is always uses its default. And as a library author I have no control over batcher and exporter.
Consider add an option to the metric constructor that allows to pass an aggregator.

(And offtopic: why ValueRecorder metric is not exported as summary? Am I doing something wrong? I use exporter-collector. And collector uses prometheusexporter)

@dyladan
Copy link
Member

dyladan commented Aug 26, 2020

I cannot believe this functionality is still missing

Please refrain from using judgmental language

The metrics sdk is currently very beta and is known to be behind the spec. We've been focusing effort on tracing recently as tracing spec moves closer to GA.

@obecny is the maintainer with the best knowledge of metrics right now. @obecny are you aware of a way to set an aggregator?

@vmarchaud
Copy link
Member

You can specify it by using a custom batcher, there is a doc here: https://github.com/open-telemetry/opentelemetry-js/blob/master/doc/batcher-api.md
I would suggest to read about the current situation (mainly about the story around Metrics Views) before making judgement.

@morigs
Copy link
Contributor Author

morigs commented Aug 26, 2020

Story for this misunderstanding. I just said what I was thinking. IMHO this is a must have feature and I'm ready to contribute if we will reach consensus on API and implementation details

@morigs
Copy link
Contributor Author

morigs commented Aug 26, 2020

@vmarchaud I trying to figure out the way to do this.
I don't judge, I was trying to said "I'm not sure is this feature really missing or I don't know about it"

In fact as a library developer I have no control over Batcher. I can only create and update metrics. Am I wrong?

@obecny
Copy link
Member

obecny commented Aug 26, 2020

MinMaxLastSumCountAggregator will be removed soon and recorder will get MinMaxSumCount and observer will get LastValue only. You can create your own batcher and then do the aggregation to whatever you need as well (@vmarchaud mentioned this already) so you are not limited to any aggregator if you don't want to. The metrics as mentioned by @dyladan still lacks of few things due to spec changes and some of the part is not finished yet (talking about spec so hard to have everything earlier :)).

@vmarchaud vmarchaud added question User is asking a question not related to a new feature or bug and removed feature-request labels Aug 26, 2020
@morigs
Copy link
Contributor Author

morigs commented Aug 26, 2020

Thanks for answers. If I understand correctly views API is what I was looking for.

May be consider creating a tracking issue? This will allow to submit a PR with draft implementation as done in the python library
open-telemetry/opentelemetry-python#596

@morigs
Copy link
Contributor Author

morigs commented Aug 27, 2020

So what is the recommended way to change the aggregator at the moment if a batcher is not available?

@AndrewGrachov
Copy link
Contributor

AndrewGrachov commented Aug 28, 2020

I use somthing like this. It's kinda hack, but, as mentioned above, metrics are in infancy

type GetBoundaries = (name: string) => number[];

export class CustomHistogramBatcher extends UngroupedBatcher {
	private getBoundariesFor: GetBoundaries;

	constructor(getboundaries: GetBoundaries) {
		super();
		this.getBoundariesFor = getboundaries;
	}

	aggregatorFor(descriptor: MetricDescriptor): Aggregator {
		if (/_histogram$/.test(descriptor.name)) {
			const boundaries = this.getBoundariesFor(descriptor.name);
			return new HistogramAggregator(boundaries);
		}
		return super.aggregatorFor(descriptor);
	}
}

		this.meter = new MeterProvider({
			exporter,
			interval: this.interval,
			batcher: new CustomHistogramBatcher(this.getHistogramBoundariesFor),
		}).getMeter(this.serviceName);

@AndrewGrachov
Copy link
Contributor

Would be nice to specify aggergator per measure, but IDK what standard will say

@vmarchaud
Copy link
Member

vmarchaud commented Aug 28, 2020

@AndrewGrachov Thats pretty much the goal of the views api which is discussed in the oteps repo.

So what is the recommended way to change the aggregator at the moment if a batcher is not available?

@morigs Not sure to follow, when is a batcher not available ? There are no other choice right now.

@morigs
Copy link
Contributor Author

morigs commented Aug 28, 2020

Ok thank you. I think this question may be closed now.
But please consider creating tracking issue for views api implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question User is asking a question not related to a new feature or bug
Projects
None yet
Development

No branches or pull requests

5 participants