Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow exporters to influence Aggregation #3762
Allow exporters to influence Aggregation #3762
Changes from 14 commits
3bb5625
eecddb7
9a195c3
6a07707
9f12d78
9d120b2
e227722
e2a8865
bf8e339
123f729
fbc1c12
a1c598a
619ec2c
09137e1
8a7100e
c94719c
44ecfb8
cbb9750
d99b832
0d0e0db
58ba395
547abf7
b4152b8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is the purpose of this to fail if someone configures a view for an exporter that doesn't support the specified temporality?
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.
Theoretically.
However, the Metrics SiG Is removing AggregationTemporality from the View API, so the purpose here is that an exporter can "handle everything" by default.
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.
AggregatedTemporality.CUMULATIVE? Just kidding (mostly) :-)
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.
ha!
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 would have thought that this method would take an instrument type as a parameter, since a given exporter might want this to vary by instrument.
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.
Interesting. Do you have an example of this (A backend/exporter that needs this level of sophistication)?
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 thought Prometheus wanted cumulative for everything but histograms, but I might very well be making that up.
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.
No, they need cumulative for everything. (Writing that spec now)
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.
You may be thinking of
Summary
? However in that case it's not DELTA, it's actually "last 10 minutes", which could be longer than the DELTA export interval.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.
ah, yeah, Summary is probably what I'm thinking of. But, I could imagine an exporter might want to impose a full view or set of views, in order to limit cardinality, etc. What would you think of handing the exporter some sort of Builder thingee that it could use to fully customize the output that is needed for a particular backend?
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 on the fence.
I think for the first version, I wish we had decided to go with ALL delta or ALL cumulative and let OTLP pick one or the other via config. Now we have an expectation of flexibility that I'm not sure where it truly belongs.
My thinking is that I expect the VIEW SDK to eventually have a "consistent across SDK" configuration file, something like:
I'm not so certain we'll be exposing that kind of configuration on a per-exporter level. While I think the choice of aggregation temporality really is an exporter decision, I find myself torn in that I think a consistent View configuration file to be more likely. (And what I suggest above is a really ugly version of what I think we'll eventually have, something halfway between prometheus-rewrite rules and otel-collector processor config).