-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
translate IntSum to Sum in otlp_wrappers #3621
Conversation
7bc4291
to
6a7f119
Compare
Tests failing:
|
I think that is a good sign, it seems that the otlp receiver executes the code :) |
74be1c4
to
e3b0710
Compare
exporter/otlpexporter/otlp_test.go
Outdated
@@ -277,7 +277,7 @@ func TestSendMetrics(t *testing.T) { | |||
assert.EqualValues(t, 0, atomic.LoadInt32(&rcv.totalItems)) | |||
|
|||
// A trace with 2 spans. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update comment?
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
e3b0710
to
9caa8a1
Compare
Please rebase |
179908a
to
e7a4dba
Compare
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
intoSum
.Link to tracking Issue: Part of #3534
Testing: Added a test to ensure both
Sum
andIntSum
are validSum
after translation.This change is built on top of #3619, will rebase once merged.