cumulative_to_delta_processor: Add Cumulative to Delta Conversion logic #4216

hossain-rayhan · 2021-07-19T07:28:39Z

Signed-off-by: Rayhan Hossain hossain.rayhan@outlook.com

Description:
This PR adds the logic for converting cumulative to delta SUM datapoints.

Link to tracking Issue: #3751

Testing:
Unit test added. Will add more to improve coverage.

Documentation: README updated.

hossain-rayhan · 2021-07-19T15:00:27Z

Hi @anuraaga and @Aneurysm9, would you please help with the review.

CC: @jmacd and @bogdandrutu some early feedback is highly appreciated.

jmacd · 2021-07-19T17:12:15Z

processor/cumulativetodeltaprocessor/processor.go

+		result := delta{value: val.(float64), prevTimestamp: timestamp}
+
+		if prev != nil {
+			deltaValue := val.(float64) - prev.RawValue.(float64)


I had to look through awsmetrics to see what the calculator does here. This looks good to me, although you might consider for monotonic cumulatives to insert a "Reset heuristic" the way Prometheus has. The current logic looks like it will deliver a negative delta when a cumulative value resets, and you might want to interpret that as a zero depending on your knowledge.

See https://github.com/lightstep/opentelemetry-prometheus-sidecar/blob/main/retrieval/series_cache.go#L331 for a similar handler used to convert Prometheus cumulatives w/o a start time to cumulatives with a start time. If you pass through similar logic first, you won't see the negative delta (for monotonic cumulatives).

Approving this PR because the implied logic fix probably belongs in awsmetrics (?).

Thanks Josh. Will check and update here.

~~Deleting this comment~~ On further review, it looks like the initial value is reported.

@jmacd in order to determine a reset we should initially use "startTime" and if that is missing (0), fallback to the prometheus way to detect reset?

processor/cumulativetodeltaprocessor/processor.go

hossain-rayhan · 2021-07-19T22:30:16Z

Hi @Aneurysm9 , thanks for your valuable comments. I pushed an update to address your comment. Would you please have another look.

processor/cumulativetodeltaprocessor/processor.go

hossain-rayhan · 2021-07-20T00:08:36Z

@bogdandrutu would you please take a look on this?

a-feld

I have a few questions about this logic, after discussion I can re-review.

a-feld · 2021-07-20T13:01:41Z

processor/cumulativetodeltaprocessor/processor.go

+							fromDataPoint := dataPoints.At(l)
+							labelMap := make(map[string]string)
+
+							fromDataPoint.LabelsMap().Range(func(k string, v string) bool {


Should LabelsMap be sorted here? Do we consider labels that arrive in a different order to be equivalent?

I am not sure. Is there any such possibility?

For sure they can come in any order since a "Map" (LabelMap) does not guarantee ordering. But you add them to another Map so there should be no problem. Correct?

Hi @a-feld, I think we are already sorting them in the awsmetrics calculator.

https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/internal/aws/metrics/metric_calculator.go#L99

a-feld · 2021-07-20T13:02:33Z

processor/cumulativetodeltaprocessor/processor.go

+								return true
+							})
+
+							result, _ := ctdp.deltaCalculator.Calculate(metric.Name(), labelMap, fromDataPoint.Value(), fromDataPoint.Timestamp().AsTime())


Since the delta calculator operates only on metric name and label map, what happens if the same metricName + labelMap is sent across resources / instrumentation libraries? Do metrics compute deltas without taking into account the resource / instrumentation library?

Good point. For my use case, I don't think there would be a scenario where the metric name and labels cannot uniquly identify a metric.

However, if thats a requirement, we can update the awsmetrics calculator sending a different PR and update this processor accordingly.

We cannot implement "logic" for our use-cases. I think this problem needs to be taken care of.

Hi @bogdandrutu , I pushed an update where we use Rsource attributes and Metric labels together to create the mapping.

As @a-feld mentioned, we had a meeting over zoom. They had couple of more requirements. They will send a follow-up PR after this to address those needs.

a-feld · 2021-07-20T13:05:31Z

processor/cumulativetodeltaprocessor/processor.go

+		result := delta{value: val.(float64), prevTimestamp: timestamp}
+
+		if prev != nil {
+			deltaValue := val.(float64) - prev.RawValue.(float64)


~~Deleting this comment~~ On further review, it looks like the initial value is reported.

a-feld

Rayhan and I had a conversation over zoom. I think New Relic has a couple of additional requirements:

Monotonic counter reset detection
Default include to all cumulative metrics. Our platform doesn't currently accept cumulative metrics natively; therefore, the user story we desire is for default inclusion.

We will be submitting a follow on PR containing many of the concepts from this implementation https://github.com/a-feld/cumulativetodeltaprocessor

For now, it would be great to push this forward. Approving on behalf on New Relic. ✅

hossain-rayhan · 2021-07-27T16:53:59Z

CC: @mxiamxia and @pxaws. We used the AWS MetricsCalculator here. We got a requirement to create the mapping key using Resource Attributes and Metric labels together. I had to make some changes in the MetricCalculator package and in some other from where you called the Calculate functions. I ran the unit tests in awscontainerinsightsreceiver and awsemfexporter to make sure they are working fine.

Expecting your review.

a-feld · 2021-07-27T16:56:09Z

@hossain-rayhan Just a reminder, there's an implementation here that I plan to push forward which doesn't require modifications to the AWS MetricsCalculator package 😄

hossain-rayhan · 2021-07-27T17:18:32Z

@hossain-rayhan Just a reminder, there's an implementation here that I plan to push forward which doesn't require modifications to the AWS MetricsCalculator package 😄

Hi @a-feld, I agree. Same time, I think adding the Resource Attributes makes the key strong to uniquely identify a metric. And I guess @bogdandrutu was asking for this anyway. If @bogdandrutu is OK to merge without this I can revert it.

@bogdandrutu as this is working fine for primary use cases, can we get this PR merged and address other needs in a separate PR (which @a-feld will send)? Waiting for your opinion.

a-feld · 2021-07-27T17:19:36Z

Agreed - I'm anxious to continue development on this processor, but waiting on this PR to merge 😄

hossain-rayhan · 2021-07-28T17:11:23Z

Reverted the last commit for utilizing Resurce Attributes to map the key as we decided to put another PR on top of this with other major changes. I believe, it would make the review process faster. This will still support the primary use case.

bogdandrutu · 2021-07-29T01:43:23Z

Needs rebase

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

This reverts commit 12d63cc31ea12dd880526b3ea2a67d2eda481487.

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>

hossain-rayhan · 2021-07-29T03:34:49Z

Hi @bogdandrutu , I rebased it. Please merge this one! Thanks.

hossain-rayhan · 2021-07-30T15:47:31Z

Hi @bogdandrutu , I rebased it. Please merge this one! Thanks.

A friendly ping @bogdandrutu

hossain-rayhan · 2021-08-02T18:20:54Z

Hi @bogdandrutu, Can we get this merged please?

hossain-rayhan · 2021-08-03T18:19:30Z

Hi @bogdandrutu, please I need you opnion on this. Can you please take a look. As per our discussion on last Wednesday's SIG meeting, we (@a-feld ) will send another PR on top of this.

Please say somthing. Can we get this merged?

hossain-rayhan · 2021-08-03T23:06:02Z

Thanks @bogdandrutu . Really appreciate your time. @a-feld can you plese create your PR whenever you are ready? Thanks.

hossain-rayhan requested review from a team and anuraaga July 19, 2021 07:28

github-actions bot assigned owais Jul 19, 2021

hossain-rayhan mentioned this pull request Jul 19, 2021

Enable cumulative to delta processor #4217

Merged

jmacd approved these changes Jul 19, 2021

View reviewed changes

Aneurysm9 reviewed Jul 19, 2021

View reviewed changes

Aneurysm9 approved these changes Jul 20, 2021

View reviewed changes

processor/cumulativetodeltaprocessor/processor.go Outdated Show resolved Hide resolved

a-feld reviewed Jul 20, 2021

View reviewed changes

a-feld approved these changes Jul 23, 2021

View reviewed changes

hossain-rayhan force-pushed the cummulative_to_delta_logic branch from a309bd6 to fc32228 Compare July 28, 2021 18:30

hossain-rayhan and others added 8 commits July 28, 2021 18:52

Add Cumulative to Delta Conversion logic

62d8f56

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>

Add dependency for internal awsmetrics module

64de598

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>

Add PR feedback- check loop index and temporality

9e1bac5

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>

convert config options once while creating the processor

63c02e3

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>

Map using Resource attributes and Metric labels together

474868e

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>

Update processor/cumulativetodeltaprocessor/processor.go

be9e823

Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>

Revert "Map using Resource attributes and Metric labels together"

4404e8d

This reverts commit 12d63cc31ea12dd880526b3ea2a67d2eda481487.

Update go.mod and go.sum for the processor

678f7ac

Signed-off-by: Rayhan Hossain <hossain.rayhan@outlook.com>

hossain-rayhan force-pushed the cummulative_to_delta_logic branch from fc32228 to 678f7ac Compare July 29, 2021 02:49

mxiamxia approved these changes Aug 2, 2021

View reviewed changes

bogdandrutu merged commit 41a7790 into open-telemetry:main Aug 3, 2021

a-feld mentioned this pull request Aug 5, 2021

Update cumulative to delta processor. #4444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cumulative_to_delta_processor: Add Cumulative to Delta Conversion logic #4216

cumulative_to_delta_processor: Add Cumulative to Delta Conversion logic #4216

hossain-rayhan commented Jul 19, 2021

hossain-rayhan commented Jul 19, 2021

jmacd Jul 19, 2021

hossain-rayhan Jul 19, 2021

a-feld Jul 20, 2021 •

edited

Loading

bogdandrutu Jul 21, 2021

hossain-rayhan commented Jul 19, 2021

hossain-rayhan commented Jul 20, 2021

a-feld left a comment

a-feld Jul 20, 2021

hossain-rayhan Jul 20, 2021

bogdandrutu Jul 21, 2021

hossain-rayhan Jul 23, 2021

a-feld Jul 20, 2021

hossain-rayhan Jul 20, 2021

bogdandrutu Jul 21, 2021

hossain-rayhan Jul 27, 2021

a-feld Jul 20, 2021 •

edited

Loading

a-feld left a comment •

edited

Loading

hossain-rayhan commented Jul 27, 2021

a-feld commented Jul 27, 2021

hossain-rayhan commented Jul 27, 2021

a-feld commented Jul 27, 2021

hossain-rayhan commented Jul 28, 2021

bogdandrutu commented Jul 29, 2021

hossain-rayhan commented Jul 29, 2021

hossain-rayhan commented Jul 30, 2021

hossain-rayhan commented Aug 2, 2021

hossain-rayhan commented Aug 3, 2021

hossain-rayhan commented Aug 3, 2021

cumulative_to_delta_processor: Add Cumulative to Delta Conversion logic #4216

cumulative_to_delta_processor: Add Cumulative to Delta Conversion logic #4216

Conversation

hossain-rayhan commented Jul 19, 2021

hossain-rayhan commented Jul 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-feld Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hossain-rayhan commented Jul 19, 2021

hossain-rayhan commented Jul 20, 2021

a-feld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-feld Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

a-feld left a comment • edited Loading

Choose a reason for hiding this comment

hossain-rayhan commented Jul 27, 2021

a-feld commented Jul 27, 2021

hossain-rayhan commented Jul 27, 2021

a-feld commented Jul 27, 2021

hossain-rayhan commented Jul 28, 2021

bogdandrutu commented Jul 29, 2021

hossain-rayhan commented Jul 29, 2021

hossain-rayhan commented Jul 30, 2021

hossain-rayhan commented Aug 2, 2021

hossain-rayhan commented Aug 3, 2021

hossain-rayhan commented Aug 3, 2021

a-feld Jul 20, 2021 •

edited

Loading

a-feld Jul 20, 2021 •

edited

Loading

a-feld left a comment •

edited

Loading