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

Update proto to latest proto commit #2027

Closed
wants to merge 1 commit into from

Conversation

bogdandrutu
Copy link
Member

Related to #2021

Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@codecov
Copy link

codecov bot commented Nov 5, 2020

Codecov Report

Merging #2027 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2027      +/-   ##
============================================
+ Coverage     84.37%   84.39%   +0.01%     
- Complexity     1879     1880       +1     
============================================
  Files           223      223              
  Lines          7585     7585              
  Branches        808      808              
============================================
+ Hits           6400     6401       +1     
  Misses          874      874              
+ Partials        311      310       -1     
Impacted Files Coverage Δ Complexity Δ
...dk/extension/trace/jaeger/sampler/RateLimiter.java 100.00% <0.00%> (+5.88%) 5.00% <0.00%> (+1.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 72c88b6...48322ba. Read the comment docs.

@jkwatson
Copy link
Contributor

jkwatson commented Nov 5, 2020

Wait! Is this a released version of the proto?

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

We should not use a non-released version of the proto

@bogdandrutu
Copy link
Member Author

@jkwatson do we?

@jkwatson
Copy link
Contributor

jkwatson commented Nov 5, 2020

@jkwatson do we?

I assert yes, we should only use released versions of the protobuf.

@gcacace
Copy link

gcacace commented Nov 5, 2020

Can we please then do a release of proto and then another one for the java package?

@jkwatson
Copy link
Contributor

jkwatson commented Nov 5, 2020

Can we please then do a release of proto and then another one for the java package?

Can you explain why this is needed so urgently? We are on a 2 week release cycle now, and do not generally intend to release patch releases unless there is an urgent bugfix.

@gcacace
Copy link

gcacace commented Nov 5, 2020

Can you explain why this is needed so urgently? We are on a 2 week release cycle now, and do not generally intend to release patch releases unless there is an urgent bugfix.

Because we need the new DoubleSummary data structure as per open-telemetry/opentelemetry-specification#1146 and open-telemetry/opentelemetry-specification#1145

@jkwatson
Copy link
Contributor

jkwatson commented Nov 5, 2020

Because we need the new DoubleSummary data structure as per open-telemetry/opentelemetry-specification#1146 and open-telemetry/opentelemetry-specification#1145

That explains what you need, but not why it is so urgent that we would be forced to do a patch release.

@jkwatson
Copy link
Contributor

jkwatson commented Nov 5, 2020

Also, just updating the protobuf won't mean that we're actually using the protobuf structures for anything. How is just updating the protobuf definitions going to help you at all?

@gcacace
Copy link

gcacace commented Nov 5, 2020

Because we are using opentelemetry-proto as transport layer for our use case.

@jkwatson
Copy link
Contributor

jkwatson commented Nov 5, 2020

Because we are using opentelemetry-proto as transport layer for our use case.

So...let me understand. You're using the java repo's published generated protobuf classes for something other than use in the SDK? That does not sound like a reason for us to do an emergency patch release. If you need these protobuf classes for something outside the use in the SDK, I recommend setting up a project that will build and publish those for you, rather than depending on this project which just happens to publish them as a part of the release process.

@jkwatson
Copy link
Contributor

jkwatson commented Nov 5, 2020

IMO, the correct solution to this problem is to have the protobuf repository publish various language bindings. Then, the opentelemetry-java project could depend on them like a normal library (rather than as a git submodule), and usages by other projects would be supported as well.

@anuraaga
Copy link
Contributor

anuraaga commented Nov 6, 2020

I agree that publishing stubs from the opentelemetry-proto repo would be ideal for contributor experience and version management.

I have sent open-telemetry/opentelemetry-proto#231 as a proposal to start to make that happen.

For reference, I've done something similar on a previous project and it works pretty well https://github.com/infostellarinc/stellarstation-api

@gcacace
Copy link

gcacace commented Nov 10, 2020

Would it be possible to create a new release of the proto with DoubleSummary support?

@jkwatson
Copy link
Contributor

Would it be possible to create a new release of the proto with DoubleSummary support?

Please post that question in the proto repo, since it's not something the java folks control.

@jkwatson
Copy link
Contributor

closing this. when the proto project cuts a release, I'll be happy to update our dependency.

@jkwatson jkwatson closed this Dec 23, 2020
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.

6 participants