Skip to content
This repository has been archived by the owner on Dec 23, 2023. It is now read-only.

implement gRPC client retry stats measures and views #2084

Merged
merged 8 commits into from Jan 24, 2022
Merged

implement gRPC client retry stats measures and views #2084

merged 8 commits into from Jan 24, 2022

Conversation

mackenziestarr
Copy link
Contributor

@mackenziestarr mackenziestarr commented Dec 6, 2021 •
edited

Based on my conversation with the grpc-java core team, the retry stats implementation of grpc-java is complete but lacks opencensus views. This PR adds them according to the measures and views outlined in the A45-retry-stats proposal.

Some open questions:

  • Given that maxAttempts maxes out at 5 in A6-client-retries should we specify a tighter set of buckets for the views (possibly [0, 5]) that use a distribution aggregation (resolved)
  • Do we need hourly and minute views for these stats? I can add them but I've left them off for now
  • I followed the views outlined in the spec but it seems GRPC_CLIENT_RETRIES_VIEW and GRPC_CLIENT_TRANSPARENT_RETRIES_VIEW could be derived from their per-call counterparts
  • I chose 0.28 for the @since annotation in the javadoc but maybe it should be the next minor version

@mackenziestarr
Copy link
Contributor Author

@jsuereth added more sensible bucket boundaries for the retry per call stats in 1c7d888 suggested in the spec

given that maxAttempts cannot be greater than 5 i'm not really sure why >= 10, >=100, >=1000 are suggested there - still these are an improvement.

@dapengzhang0
Copy link

@mackenziestarr

added more sensible bucket boundaries for the retry per call stats in 1c7d888 suggested in the spec

given that maxAttempts cannot be greater than 5 i'm not really sure why >= 10, >=100, >=1000 are suggested there - still these are an improvement.

Did you notice the notice in

https://github.com/grpc/proposal/blob/2d7ecf48d9a6aff7973a909be9b6552aaeafc9d6/A6-client-retries.md#retry-and-hedging-statistics

Notice: Retry statistics has been updated in the new design gRFC-A45. The original design below is obsolete.

@mackenziestarr
Copy link
Contributor Author

mackenziestarr commented Jan 5, 2022 •
edited

Yes, i did notice it. Is there a better alternative you would suggest? I didn't see bucket boundaries specified in gRFC-A45.

@dapengzhang0
Copy link

dapengzhang0 commented Jan 5, 2022 •
edited

It does not make sense to have >= 10, >=100, >=1000 buckets for count of retry-attempts. Those buckets were addd in the very first version of gRFC-A6, and later gRFC-A6 imposed maxRetryAttempts<=5 but the buckets were not updated (probably overlooked). See ncteisen/proposal@7354c1a#diff-a1d8d0295ab02319b3f5cb1f351fc16727eff4a614373d11eef437c89efb35fbL178-R177

@mackenziestarr
Copy link
Contributor Author

ah okay makes sense. I'll remove the extraneous buckets and make >= 5 the upper bound - does that sound good?

@dapengzhang0
Copy link

ah okay makes sense. I'll remove the extraneous buckets and make >= 5 the upper bound - does that sound good?

Yep.

@mackenziestarr
Copy link
Contributor Author

thanks @dapengzhang0 fixed in e597fe6

@mackenziestarr
Copy link
Contributor Author

hi @jsuereth would you mind approving again (pending any changes that should be made) so CI can run?

asafdav2
asafdav2 reviewed Jan 18, 2022
View reviewed changes
contrib/grpc_metrics/src/main/java/io/opencensus/contrib/grpc/metrics/RpcViewConstants.java Outdated
View.create(
View.Name.create("grpc.io/client/retries_per_call"),
"Number of client retries per call",
GRPC_CLIENT_TRANSPARENT_RETRIES_PER_CALL,
Copy link

Choose a reason for hiding this comment

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

should this be GRPC_CLIENT_RETRIES_PER_CALL ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes great catch, thanks @asafdav2 - updated

@mackenziestarr
Copy link
Contributor Author

@songy23 @rghetia can I get a review on this when you get a chance, thanks

@punya
Copy link
Contributor

punya commented Jan 24, 2022

@mackenziestarr I work on the same team as @jsuereth and I plan to review/approve, merge and release this PR this week. Thanks for your patience!

@punya punya self-assigned this Jan 24, 2022
punya
punya suggested changes Jan 24, 2022
View reviewed changes
Copy link
Contributor

@punya punya left a comment

Choose a reason for hiding this comment

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

@dapengzhang0 would you mind reviewing this and indicating your approval from a gRPC perspective?

From an OpenCensus perspective, I am on board with the change - I just have a couple of suggested edits to fix @since comments.

contrib/grpc_metrics/src/main/java/io/opencensus/contrib/grpc/metrics/RpcMeasureConstants.java Outdated Show resolved Hide resolved
contrib/grpc_metrics/src/main/java/io/opencensus/contrib/grpc/metrics/RpcViews.java Outdated Show resolved Hide resolved
dapengzhang0
dapengzhang0 approved these changes Jan 24, 2022
View reviewed changes
Copy link

@dapengzhang0 dapengzhang0 left a comment

Choose a reason for hiding this comment

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

LGTM

@punya punya merged commit 2e90f49 into census-instrumentation:master Jan 24, 2022
@mackenziestarr
Copy link
Contributor Author

thanks for updating those @punya, appreciate it 😄

gcf-merge-on-green bot pushed a commit to googleapis/java-spanner that referenced this pull request Feb 2, 2022
@renovate-bot
deps: update opencensus.version to v0.31.0 (#1661)
1e86a3a 
[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [io.opencensus:opencensus-contrib-grpc-metrics](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-grpc-metrics/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-grpc-metrics/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-grpc-metrics/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-grpc-metrics/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |
| [io.opencensus:opencensus-exporter-stats-stackdriver](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-exporter-stats-stackdriver/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-exporter-stats-stackdriver/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-exporter-stats-stackdriver/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-exporter-stats-stackdriver/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |
| [io.opencensus:opencensus-exporter-trace-stackdriver](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-exporter-trace-stackdriver/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-exporter-trace-stackdriver/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-exporter-trace-stackdriver/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-exporter-trace-stackdriver/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |
| [io.opencensus:opencensus-contrib-zpages](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-zpages/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-zpages/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-zpages/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-zpages/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |
| [io.opencensus:opencensus-impl](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-impl/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-impl/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-impl/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-impl/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |
| [io.opencensus:opencensus-api](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-api/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-api/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-api/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-api/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |
| [io.opencensus:opencensus-contrib-grpc-util](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-grpc-util/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-grpc-util/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-grpc-util/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-grpc-util/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>census-instrumentation/opencensus-java</summary>

### [`v0.31.0`](https://togithub.com/census-instrumentation/opencensus-java/releases/v0.31.0)

[Compare Source](https://togithub.com/census-instrumentation/opencensus-java/compare/v0.30.0...v0.31.0)

-   fix: Shutdown Stackdriver MetricServiceClient properly by [@&#8203;janhicken](https://togithub.com/janhicken) in [census-instrumentation/opencensus-java#2091
-   implement gRPC client retry stats measures and views by [@&#8203;mackenziestarr](https://togithub.com/mackenziestarr) in [census-instrumentation/opencensus-java#2084

**Full Changelog**: census-instrumentation/opencensus-java@v0.29.0...v0.31.0

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-spanner).
gcf-merge-on-green bot pushed a commit to googleapis/google-http-java-client that referenced this pull request Feb 9, 2022
@renovate-bot
deps: update project.opencensus.version to v0.31.0 (#1563)
0f9d2b7 
[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [io.opencensus:opencensus-testing](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-testing/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-testing/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-testing/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-testing/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |
| [io.opencensus:opencensus-impl](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-impl/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-impl/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-impl/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-impl/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |
| [io.opencensus:opencensus-contrib-http-util](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-http-util/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-http-util/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-http-util/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-contrib-http-util/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |
| [io.opencensus:opencensus-api](https://togithub.com/census-instrumentation/opencensus-java) | `0.30.0` -> `0.31.0` | [![age](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-api/0.31.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-api/0.31.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-api/0.31.0/compatibility-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/io.opencensus:opencensus-api/0.31.0/confidence-slim/0.30.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>census-instrumentation/opencensus-java</summary>

### [`v0.31.0`](https://togithub.com/census-instrumentation/opencensus-java/releases/v0.31.0)

[Compare Source](https://togithub.com/census-instrumentation/opencensus-java/compare/v0.30.0...v0.31.0)

-   fix: Shutdown Stackdriver MetricServiceClient properly by [@&#8203;janhicken](https://togithub.com/janhicken) in [census-instrumentation/opencensus-java#2091
-   implement gRPC client retry stats measures and views by [@&#8203;mackenziestarr](https://togithub.com/mackenziestarr) in [census-instrumentation/opencensus-java#2084

**Full Changelog**: census-instrumentation/opencensus-java@v0.29.0...v0.31.0

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/google-http-java-client).
@mackenziestarr mackenziestarr mentioned this pull request Feb 11, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@asafdav2 asafdav2 asafdav2 left review comments

@punya punya punya approved these changes

@dapengzhang0 dapengzhang0 dapengzhang0 approved these changes

@rghetia rghetia Awaiting requested review from rghetia

@songy23 songy23 Awaiting requested review from songy23

@jsuereth jsuereth Awaiting requested review from jsuereth

Assignees

@punya punya

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mackenziestarr @dapengzhang0 @punya @jsuereth @asafdav2