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.

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

Calculate delta instead of rate for cumulative metrics in EMF exporter #2512

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

bjrara
Copy link
Contributor

@bjrara bjrara commented Mar 1, 2021

Why do we need it?
This is a follow-up PR of #2627.

Currently, when cumulative metrics, e.g. counter/summary, come into EMF exporter, the value will be adjusted to rate (in seconds). However, this is not the correct behavior, as what we observed from CloudWatch dashboard, the backend expects delta calculation result.

In this PR, we change the rate calculation to delta regarding cumulative metrics.

@bjrara bjrara requested a review from a team March 1, 2021 06:57
@bjrara
Copy link
Contributor Author

bjrara commented Mar 1, 2021

/assign @mxiamxia

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #2512 (79a5e5a) into main (359d6e4) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2512      +/-   ##
==========================================
- Coverage   91.60%   91.59%   -0.01%     
==========================================
  Files         450      450              
  Lines       22191    22183       -8     
==========================================
- Hits        20327    20319       -8     
  Misses       1394     1394              
  Partials      470      470              
Flag Coverage Δ
integration 69.09% <ø> (-0.07%) ⬇️
unit 90.53% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
exporter/awsemfexporter/datapoint.go 100.00% <100.00%> (ø)
exporter/awsemfexporter/grouped_metric.go 100.00% <100.00%> (ø)
exporter/awsemfexporter/metric_translator.go 97.94% <100.00%> (+0.02%) ⬆️
internal/aws/metric_calculator.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 359d6e4...79a5e5a. Read the comment docs.

@bjrara bjrara force-pushed the delta_counter branch 7 times, most recently from a3be34d to c10d724 Compare March 2, 2021 03:38
@bjrara bjrara changed the title [AWS] Calculate delta instead of rate for cumulative metrics [WIP] [AWS] Calculate delta instead of rate for cumulative metrics Mar 3, 2021
@bjrara bjrara changed the title [WIP] [AWS] Calculate delta instead of rate for cumulative metrics [AWS] Calculate delta instead of rate for cumulative metrics Mar 3, 2021
@bjrara bjrara changed the title [AWS] Calculate delta instead of rate for cumulative metrics [WIP] [AWS] Calculate delta instead of rate for cumulative metrics Mar 5, 2021
@bjrara bjrara changed the title [WIP] [AWS] Calculate delta instead of rate for cumulative metrics [AWS] Calculate delta instead of rate for cumulative metrics Mar 8, 2021
@bjrara bjrara changed the title [AWS] Calculate delta instead of rate for cumulative metrics Calculate delta instead of rate for cumulative metrics in EMF exporter Mar 8, 2021
@anuraaga
Copy link
Contributor

Is it possible to do the refactoring, moving the files around, first in a PR and followup with the logic change to make it easier to review?

@bjrara
Copy link
Contributor Author

bjrara commented Mar 10, 2021

Is it possible to do the refactoring, moving the files around, first in a PR and followup with the logic change to make it easier to review?

Split to 2 PRs: #2627

@bjrara bjrara force-pushed the delta_counter branch 3 times, most recently from 064f076 to eaad1bf Compare March 12, 2021 18:04
kisieland referenced this pull request in kisieland/opentelemetry-collector-contrib Mar 16, 2021
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on reviewing this, I got confused on which PRs are doing what. Good to make dependent PRs as draft while dependencies are waiting

exporter/awsemfexporter/datapoint.go Outdated Show resolved Hide resolved
@bjrara bjrara force-pushed the delta_counter branch 3 times, most recently from a75bf4b to 9d4d8f7 Compare March 19, 2021 00:07
@bjrara bjrara force-pushed the delta_counter branch 3 times, most recently from eabae34 to 00ebaf0 Compare March 24, 2021 01:44
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

Thanks!

@mxiamxia please do another review too

@bjrara
Copy link
Contributor Author

bjrara commented Mar 24, 2021

Thanks!

@mxiamxia please do another review too

Thank you!

Copy link
Member

@mxiamxia mxiamxia left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@tigrannajaryan tigrannajaryan merged commit 8b2350d into open-telemetry:main Mar 25, 2021
pmatyjasek-sumo referenced this pull request in pmatyjasek-sumo/opentelemetry-collector-contrib Apr 28, 2021
**Why do we need it?**
This is a follow-up PR of open-telemetry#2627.

Currently,  when cumulative metrics, e.g. counter/summary, come into EMF exporter, the value will be adjusted to rate (in seconds). However, this is not the correct behavior, as what we observed from CloudWatch dashboard, the backend expects delta calculation result.

In this PR, we change the rate calculation to delta regarding cumulative metrics.
@bjrara bjrara deleted the delta_counter branch May 12, 2021 18:39
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
#2512)

* emitBatchOverhead should only be used for splitting spans into batches (#2503)

* limit max packet size parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants