[exporter/elasticsearch] Add explicit bounds histogram support to metrics #34045
Conversation
carsonip
changed the title
| err := exporter.ConsumeMetrics(context.Background(), metrics) | ||
| require.ErrorContains(t, err, "invalid histogram data point") | ||
| require.ErrorContains(t, err, "invalid number data point") |
There was a problem hiding this comment.
[to reviewer] ConsumeMetrics will return an error, I don't think we can return a partial error here. The valid metric data points will be published, and invalid ones will be dropped.
There was a problem hiding this comment.
I'm wondering if you can elaborate on this. Is there a reason why you cannot use a PartialSuccess and indicate the number of points rejected?
There was a problem hiding this comment.
It looks like it handles errors the same for histograms as it does for other metric types, so, this is probably irrelevant.
There was a problem hiding this comment.
pushMetrics returns an error, but PartialSuccess is not an error. I am not aware of a way to meaningfully return to caller to indicate a partial success in an exporter. Even in otelarrowexporter it returns a nil error if partial success happens.
The handling in this PR takes a different route. If it drops any data points, it returns an explicit error. We may revisit in the future whether this is desirable.
Co-authored-by: Andrew Wilkins <[email protected]>
Thanks for the reminder about dynamic template in bulk request. Yes, we'll need it, but the scope is a bit larger / different to this PR. I'll do it in a follow up.
It is the test in exporter_test, which works e2e from an exporter PoV, not exactly e2e collector test. I've updated the PR description to reflect that. |
carsonip
changed the title
Description:
Add explicit bounds histogram support to metrics
Link to tracking Issue:
Testing:
Added test in exporter_test
Documentation: