Change prometheus to not aggregate metrics and only export them. #385
Conversation
Measures are being exported as summaries since histograms doesn't exist on OpenTelemetry yet. Better error handling must be done.
There was a problem hiding this comment.
I believe there's a synchronization issue here--I think it can be addressed by simply copying the checkpoint set.
I also am still pushing for a new kind of "pull" controller (which may be dedicated to Prometheus only) that would call Collect() on demand as I think it will make the synchronization more natural and address the need to reset some state, discussed below.
That said, I would be happy to approve this and work on the remaining issues in a future PR.
jmacd
requested review from
iredelmeier,
lizthegrey,
rghetia and
tedsuo
as code owners
|
remove WIP from title if ready for review. |
paivagustavo
changed the title
exporter/metric/test/test.go
Outdated
| _ = gagg.Update(ctx, createNumber(desc, v), desc) | ||
| gagg.Checkpoint(ctx, desc) | ||
| p.Add(desc, gagg, labels...) | ||
| if agg != gagg { |
There was a problem hiding this comment.
Can you add a comment about this new test support?
There was a problem hiding this comment.
Sure, this was a little bit enigmatic. I've refactored it a bit and added some docs.
|
@lizthegrey are you satisfied with the changes? @jmacd yesterday you had mentioned that you were going to look at this PR one more time. Do you have additional comments? |
|
I just wanted to make sure I understood the test changes (and I do). Thanks @paivagustavo! |
Closes #370
What does this changes?
There is an obvious improvement to be done here in the error handling part.
With this changes, The
Export(_ context.Context, checkpointSet export.CheckpointSet)only stores the lastcheckpointSetand defer the exporting to when the prometheus http handler is called.Initially I thought about a bigger change, using a pull controller that would aggregate data when this handler is activated, but that could make the export operation slower.
The decision here was between two types of exporters:
Controller.Ticks)I've chosen the later to have a more consistent and ligther operation when exporting data. After further analyses of the OpenCensus they also do it this way.
I've also changed the
test.CheckpointSetto accept Add several metrics with the same name and label set, this would batch them just like adefaultkeys.Batcherto proper tests summaries/histogram since Prometheus doesn't aggregate them anymore.