Skip to content

add exemplars support to prometheusremotewriteexporter #5578

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Nov 23, 2021
Merged

add exemplars support to prometheusremotewriteexporter #5578

merged 8 commits into from
Nov 23, 2021

Conversation

anneelisabethlelievre
Copy link
Contributor

@anneelisabethlelievre anneelisabethlelievre commented Oct 4, 2021 •
edited
Loading

Signed-off-by: Anne-Elisabeth Lelievre [email protected]

Hello, this is the implementation of #5192 to support exemplars for the prometheus remote write exporter.

Thanks

@anneelisabethlelievre anneelisabethlelievre marked this pull request as ready for review October 4, 2021 15:48
@anneelisabethlelievre anneelisabethlelievre requested review from a team and codeboten October 4, 2021 15:48
@anneelisabethlelievre
fix lint error
9ddb1c4 
Signed-off-by: Anne-Elisabeth Lelievre <[email protected]>
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 12, 2021
@secat
Copy link

secat commented Oct 12, 2021

@codeboten I think this PR is ready for review

@codeboten codeboten removed the Stale label Oct 12, 2021
codeboten
codeboten reviewed Oct 12, 2021
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few questions. It would be great to get a second review from one of the awsprometheusremotewriteexporter codeowners.

Ping @alolita @anuraaga @rakyll

anuraaga
anuraaga reviewed Oct 13, 2021
View reviewed changes
exporter/prometheusremotewriteexporter/helper.go Outdated
infLabels := createAttributes(resource, pt.Attributes(), externalLabels, nameStr, baseName+bucketStr, leStr, pInfStr)
addSample(tsMap, infBucket, infLabels, metric)

promExemplar := getPromExemplar(pt, len(pt.BucketCounts())-1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify where len(pt.BucketCounts())-1 comes from? I could be misunderstanding it, but my reading of the proto doesn't indicate any relationship between bucket counts and exemplars

https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/metrics/v1/metrics.proto#L473

Shouldn't we just go through all the exemplars, convert them and add them all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anuraaga according to my understanding, the len(pt.BucketCounts())-1 seems to be the index for the +inf bucket. For me, from what I read, it seems to exists a relationship between the bucket index and the exemplars with the support in prometheus:

prometheus/prometheus#9414

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying! Can you add a comment referencing that we need to use the +inf bucket?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes of course! done

@anneelisabethlelievre
fix according to review
271f240 
Signed-off-by: Anne-Elisabeth Lelièvre <[email protected]>
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 18, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

anneelisabethlelievre and others added 2 commits October 18, 2021 11:43
@anneelisabethlelievre
add comment in helper_test
bae21d1 
Signed-off-by: Anne-Elisabeth Lelievre <[email protected]>
@anneelisabethlelievre
fix tests
f533ada 
Signed-off-by: Anne-Elisabeth Lelièvre <[email protected]>
codeboten
codeboten approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my comments, @anuraaga please review and approve if your comments have been addressed.