-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[chore] [exporter/elasticsearch] Extract datapoints into their own internal package #37237
base: main
Are you sure you want to change the base?
Conversation
f2a7655
to
af44849
Compare
af44849
to
36b6349
Compare
51fbdaf
to
71ec21d
Compare
|
||
func histogramToValue(dp pmetric.HistogramDataPoint) (pcommon.Value, error) { | ||
// Histogram conversion function is from | ||
// https://github.com/elastic/apm-data/blob/3b28495c3cbdc0902983134276eb114231730249/input/otlp/metrics.go#L277 |
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 there no alternative to duplicating the logic here? Is there any way to make the original functionality public? Would it be helpful to at least add a link in the histogramSample
method that's being duplicated to update this method whenever it changes?
I see this method is simply being copied, no need to block this PR, just thought I'd call it out.
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.
Possibly. I'm improving the overall readability and separation of the module, and it's already causing frequent conflicts with other changes. So'id rather avoid mixing refactorings into it.
Description
This PR is part of the splitting of the elasticsearch exporter for a better separation of concerns.
It follows (and is currently based after) #37235.
This introduces a new datapoints package, so multiple mapping type packages can rely on them.