Skip to content
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

Remove delta adjustment from summaries by default in EMF exporter #3408

Merged
merged 1 commit into from
May 21, 2021
Merged

Remove delta adjustment from summaries by default in EMF exporter #3408

merged 1 commit into from
May 21, 2021

Conversation

bjrara
Copy link
Contributor

@bjrara bjrara commented May 17, 2021 •
edited
Loading

Why do we need it?
Fixes aws-observability/aws-otel-collector#510.
This is a replacement PR for #3373.

In CloudWatch, we expect metrics to be sent as delta so the optimization of deltas can be fully utilized.

Currently, in prometheus receiver, the count and sum are not delta:

The sum and count from Summary are cumulative, however, the quantiles are not. The receiver will again maintain some state to attempt to detect value resets and to set appropriate start timestamps.

On the other hand, in OTEL SDKs, the same type is calculated as delta.

This PR checks whether metrics are tagged with prometheus attribute to decide whether EMF exporter should calculate delta for summaries.

@bjrara bjrara requested a review from anuraaga as a code owner May 17, 2021 20:29
@bjrara bjrara requested a review from a team May 17, 2021 20:29
@bjrara
Copy link
Contributor Author

bjrara commented May 17, 2021

/assign @hossain-rayhan
/assign @mxiamxia

hossain-rayhan
hossain-rayhan reviewed May 17, 2021
View reviewed changes
Copy link
Contributor

@hossain-rayhan hossain-rayhan left a comment

Choose a reason for hiding this comment

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

Looks good. I would expect two different unit tests- one with prometheus-receiver and the other without prometheus-receiver.

mxiamxia
mxiamxia approved these changes May 17, 2021
View reviewed changes
Copy link
Member

@mxiamxia mxiamxia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM and Thanks!

@bjrara
Copy link
Contributor Author

bjrara commented May 17, 2021

Looks good. I would expect two different unit tests- one with prometheus-receiver and the other without prometheus-receiver.

Done. Thanks for the advice.

@bjrara
Copy link
Contributor Author

bjrara commented May 17, 2021

May I know why build-publish job fails instantly after new commit submitted? Is it caused by windows-test - Unauthorized?

hossain-rayhan
hossain-rayhan approved these changes May 17, 2021
View reviewed changes
Copy link
Contributor

@hossain-rayhan hossain-rayhan left a comment

Choose a reason for hiding this comment

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

Thanks.

@bjrara bjrara force-pushed the emf branch 4 times, most recently from dd149aa to 1d3b100 Compare May 19, 2021 19:23
tigrannajaryan
tigrannajaryan approved these changes May 19, 2021
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

LGTM. Merge after the build passes.

@bogdandrutu bogdandrutu merged commit eb5d360 into open-telemetry:main May 21, 2021
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
@odeke-em
receiver/prometheus: reject datapoints with duplicate label keys (#3408)
933504a 
As of Prometheus 2.16.0 (released on 2020-02-13), datapoints with
duplicate label keys MUST be rejected and the Prometheus RemoteWrite
exporter tests were failing, but that was a red herring as the
real issue was really in the receiver.

Fixes open-telemetry/prometheus-interoperability-spec#44
Fixes #3407
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mxiamxia mxiamxia mxiamxia approved these changes

@hossain-rayhan hossain-rayhan hossain-rayhan approved these changes

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@anuraaga anuraaga Awaiting requested review from anuraaga

Assignees

@owais owais

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Export raw values for cumulative metrics in EMF Exporter
6 participants
@bjrara @tigrannajaryan @hossain-rayhan @mxiamxia @owais @bogdandrutu