census-instrumentation / opencensus-java

A stats collection and distributed tracing framework
https://opencensus.io
Apache License 2.0
672 stars 201 forks source link

Fix retry stat measures to match those in grpc-java exactly #2097

Closed mackenziestarr closed 2 years ago

mackenziestarr commented 2 years ago

In the medium term grpc-java should reference the retry stat Measures defined in opencensus-java instead of creating its own, although these are referenced in a file called DeprecatedCensusConstants.java so i'm not sure what the long-term goal there is.

mackenziestarr commented 2 years ago

cc: @punya @dapengzhang0

punya commented 2 years ago

Would it be possible to add a test that verifies that the measures match?

@dapengzhang0 do you know why the measure constants are considered deprecated on the OpenCensus side? E.g., was there a desire at some point for the definitions in this repo to be considered the source of truth?

mackenziestarr commented 2 years ago

hey @punya added a test in 05e8256, hopefully it is what you had in mind.

punya commented 2 years ago

hey @punya added a test in 05e8256, hopefully it is what you had in mind.

Thanks for putting in the effort to write this. I was actually hoping that the test would check against the source of truth (grpc-java), not against hardcoded constants which simply repeat the code. If accessing the source of truth is impossible (or even very kludgey), I'd rather revert this test commit and merge without it.

dapengzhang0 commented 2 years ago

@dapengzhang0 do you know why the measure constants are considered deprecated on the OpenCensus side? E.g., was there a desire at some point for the definitions in this repo to be considered the source of truth?

@punya, that might be for some historical reasons mentioned in the comment here https://github.com/grpc/grpc-java/pull/4494#discussion_r190654824

I don't if the current status inside google has changed. cc\ @zhangkun83 who might have more context.

mackenziestarr commented 2 years ago

@punya I think any efforts towards preventing a regression would better be spent opening a pull request in grpc-java to upgrade to the eventual opencensus version that contains this commit and reference the measures defined here directly - I'd be happy to contribute that once this is merged. I'm fine with you reverting the superfluous unit test, I agree it adds nothing.

mackenziestarr commented 2 years ago

hi @punya would it be possible to cut this into a 0.31.1 release since its a backwards compatible bug fix

punya commented 2 years ago

@mackenziestarr I'd be happy to, but probably no sooner than next week (unfortunately the release process isn't as automated as we'd like it to be). Would that work for you?

mackenziestarr commented 2 years ago

@punya sounds great! thanks for responding so quickly

mackenziestarr commented 2 years ago

hi @punya would it be possible to cut a release this week or the next?

punya commented 2 years ago

@mackenziestarr my apologies for dropping the ball on this. I'll get this out today.

mackenziestarr commented 2 years ago

no problem @punya, thank you!

punya commented 2 years ago

FYI: I ran into a few CI snags, but I'm still working on this. We haven't done a patch release in a while.

mackenziestarr commented 2 years ago

hi @punya is the patch release still blocked on CI?

punya commented 2 years ago

Sorry for the long wait, @mackenziestarr. This release is finally out (Github | Sonatype).

mackenziestarr commented 2 years ago

@punya no worries, thank you! i just pulled down the release artifact and tested it out, works great