Fix retry stat measures to match those in grpc-java exactly #2097
Conversation
…ctly change GRPC_CLIENT_RETRY_DELAY_PER_CALL from MeasureLong to MeasureDouble
mackenziestarr
requested review from
a team,
rghetia and
songy23
as code owners
|
cc: @punya @dapengzhang0 |
|
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? |
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. |
@punya, that might be for some historical reasons mentioned in the comment here I don't if the current status inside google has changed. cc\ @zhangkun83 who might have more context. |
|
@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. |
…scriptors" This reverts commit 05e8256.
|
hi @punya would it be possible to cut this into a |
|
@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? |
|
@punya sounds great! thanks for responding so quickly |
|
hi @punya would it be possible to cut a release this week or the next? |
|
@mackenziestarr my apologies for dropping the ball on this. I'll get this out today. |
…nstrumentation#2097) * update descriptions of retry measures to match those in grpc-java exactly * change GRPC_CLIENT_RETRY_DELAY_PER_CALL from MeasureLong to MeasureDouble
|
no problem @punya, thank you! |
…2102) * update descriptions of retry measures to match those in grpc-java exactly * change GRPC_CLIENT_RETRY_DELAY_PER_CALL from MeasureLong to MeasureDouble Co-authored-by: Mackenzie Starr <[email protected]>
|
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. |
|
hi @punya is the patch release still blocked on CI? |
|
Sorry for the long wait, @mackenziestarr. This release is finally out (Github | Sonatype). |
|
@punya no worries, thank you! i just pulled down the release artifact and tested it out, works great |
GRPC_CLIENT_RETRY_DELAY_PER_CALLfromMeasureLongtoMeasureDoubleIn 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.