Update opentelemetry-proto to latest version. #1600
Conversation
anuraaga
requested review from
arminru,
bogdandrutu,
carlosalberto,
jkwatson,
pavolloffay,
thisthat and
tylerbenson
as code owners
| .setPercentile(valueAtPercentile.getPercentile()) | ||
| .setValue(valueAtPercentile.getValue()) | ||
| .build()); | ||
| // TODO(jkwatson): Value of histogram should be long? |
There was a problem hiding this comment.
I have no remembrance of what this might have meant.
Codecov Report
@@ Coverage Diff @@
## master #1600 +/- ##
============================================
- Coverage 91.56% 91.50% -0.06%
Complexity 985 985
============================================
Files 117 117
Lines 3603 3603
Branches 314 314
============================================
- Hits 3299 3297 -2
- Misses 208 209 +1
- Partials 96 97 +1
Continue to review full report at Codecov.
|
|
What version of the proto is this updated to? I haven't seen a release since 0.4.0. |
|
Just latest master 32a0c7b519d6e26749a44d7ca8b5a39b08d9ef00 Collector seems to track master so I don't know if the proto has a real release process. But if collector moves with master I think SDK needs to as well to keep up. |
|
I would prefer not to try to chase master with this. I think there are still changes happening rapidly to the metrics protobuf definitions. I would greatly prefer to only used released versions of the proto. |
+1 on this. |
|
@bogdandrutu Will you be doing a proto release soon? We should unblock the trace proto incompatibility soon. |
|
Release will happen Monday, you only need to resync and regenerate the code because of open-telemetry/opentelemetry-proto#214, but Java code will not change |
|
JFYI, I removed the dependency on go / java being protocol compatible in the instrumentation repo's smoke tests so we don't have a rush on fixing this with regards to instrumentation. |
|
0.5.0 was just released: https://github.com/open-telemetry/opentelemetry-proto/releases/tag/v0.5.0 So, please update and we'll get this into the 0.8.0 release tomorrow! |
|
@jkwatson hopefully the collector is changed by tonight so we can give a try before releasing |
If not, I think we're ok delaying a day. Should we hold off on merging this, after it's been updated for 0.5.0, do you think? |
|
Before releasing this change I would give a try to the collector. I am getting very close to finish that, is under review. After that gets merged will announce on the metrics gitter |
|
All collector code is merged, waiting for the CI to publish a docker image for that build |
|
Updated submodule to match the tag version. |
|
Should we merge this, or can you test it against the latest collector before merging, @anuraaga ? |
|
I haven't really been using metrics yet it'd be quite a chore for me to test it. Maybe merge and give a chance to allow someone (@bogdandrutu ?) to test with the snapshot before releasing? |
Currently, java instrumentation smoke tests fail because we use JSON. collector
fileexporter writes as JSON and the tests read the JSON. But currently the JSON is not compatible between collector and Java because of renamed enums. Updating the proto will allow us to continue to develop against new versions of the collector.I'm guessing the intent of the metrics protocol change is for the SDK to also adopt similar data structures. I'm not doing that in this PR since I only want to make the SDK JSON-compatible with the latest collectors - at the same time I've probably made them somewhat compatible for metrics too. The conversion of summary to histogram is clearly not ideal, converting double values into long counts, but I think semantically it may capture something reasonable. Anyways given before the change, the SDK isn't compatible at all for metrics with the latest collector, this is probably an improvement until rewriting the SDK.