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

translate IntSum to Sum in otlp_wrappers #3621

Merged
merged 2 commits into from
Jul 16, 2021

Conversation

codeboten
Copy link
Contributor

Description:
Deprecating IntSum As per https://github.com/open-telemetry/opentelemetry-proto/blob/f3b0ee0861d304f8f3126686ba9b01c106069cb0/opentelemetry/proto/metrics/v1/metrics.proto#L156, the OTLP receiver will translate IntSum into Sum.

Link to tracking Issue: Part of #3534

Testing: Added a test to ensure both Sum and IntSum are valid Sum after translation.

This change is built on top of #3619, will rebase once merged.

@codeboten codeboten marked this pull request as ready for review July 14, 2021 20:18
@codeboten codeboten requested review from a team and dmitryax July 14, 2021 20:18
@bogdandrutu
Copy link
Member

Tests failing:

=== RUN   TestSendMetrics
    otlp_test.go:295: 
        	Error Trace:	otlp_test.go:295
        	Error:      	Not equal: 
        	            	expected: pdata.Metrics{orig:(*v1.ExportMetricsServiceRequest)(0xc00000e380)}
        	            	actual  : pdata.Metrics{orig:(*v1.ExportMetricsServiceRequest)(0xc0003aa860)}
        	            	
        	            	Diff:
        	Test:       	TestSendMetrics

@bogdandrutu
Copy link
Member

I think that is a good sign, it seems that the otlp receiver executes the code :)

@@ -277,7 +277,7 @@ func TestSendMetrics(t *testing.T) {
assert.EqualValues(t, 0, atomic.LoadInt32(&rcv.totalItems))

// A trace with 2 spans.
Copy link
Member

Choose a reason for hiding this comment

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

Update comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

@@ -277,7 +277,7 @@ func TestSendMetrics(t *testing.T) {
assert.EqualValues(t, 0, atomic.LoadInt32(&rcv.totalItems))

// A trace with 2 spans.
md = testdata.GenerateMetricsTwoMetrics()
md = testdata.GenerateMetricsTwoSumMetrics()
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding new method maybe consider to change the other one and add same TODO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought adding another func would be less invasive, but it turns out I was wrong. Will add the same TODO to all the places I change here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bogdandrutu I ended up adding 2 temporary test methods, changing the existing one caused all sorts of problems with other tests that were depending on them.

I plan on tackling the IntDataPoint/DoubleDataPoint conversion to NumberDataPoint soon, which will allow us to remove these temporary functions.

@bogdandrutu
Copy link
Member

Please rebase

@bogdandrutu bogdandrutu merged commit 22496b7 into open-telemetry:main Jul 16, 2021
@codeboten codeboten deleted the codeboten/deprecate-intsum branch June 22, 2023 15:17
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.

2 participants