FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

implement gRPC client retry stats measures and views by mackenziestarr · Pull Request #2084 · census-instrumentation/opencensus-java · GitHub

This repository was archived by the owner on Dec 23, 2023. It is now read-only.
/ opencensus-java Public archive

implement gRPC client retry stats measures and views - #2084

Merged
punya merged 8 commits into
census-instrumentation:masterfrom
mackenziestarr:mstarr/grpc-retry-stats
Jan 24, 2022
ConversationCommits (8)ChecksFiles changed
Merged

implement gRPC client retry stats measures and views#2084
punya merged 8 commits into
census-instrumentation:masterfrom
mackenziestarr:mstarr/grpc-retry-stats

Conversation

mackenziestarr commented Dec 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 requested review from a team, rghetia and songy23 as code owners December 6, 2021 22:30

mackenziestarr commented Jan 5, 2022

Copy link
Copy Markdown
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 commented Jan 5, 2022

Copy link
Copy Markdown

@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 commented Jan 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

dapengzhang0 commented Jan 5, 2022 •
edited
Loading

Copy link
Copy Markdown

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 commented Jan 5, 2022

Copy link
Copy Markdown
Contributor Author

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

dapengzhang0 commented Jan 5, 2022

Copy link
Copy Markdown

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

Yep.

mackenziestarr commented Jan 5, 2022

Copy link
Copy Markdown
Contributor Author

thanks @dapengzhang0 fixed in e597fe6

mackenziestarr commented Jan 11, 2022

Copy link
Copy Markdown
Contributor Author

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

asafdav2 reviewed Jan 18, 2022
View reviewed changes
Comment thread 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,

asafdav2 Jan 18, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

should this be GRPC_CLIENT_RETRIES_PER_CALL ?

mackenziestarr Jan 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes great catch, thanks @asafdav2 - updated

mackenziestarr commented Jan 24, 2022

Copy link
Copy Markdown
Contributor Author

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

punya commented Jan 24, 2022

Copy link
Copy Markdown
Contributor

@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 self-assigned this Jan 24, 2022
punya suggested changes Jan 24, 2022
View reviewed changes

punya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

Comment thread contrib/grpc_metrics/src/main/java/io/opencensus/contrib/grpc/metrics/RpcMeasureConstants.java Outdated
Comment thread contrib/grpc_metrics/src/main/java/io/opencensus/contrib/grpc/metrics/RpcViews.java Outdated
dapengzhang0 approved these changes Jan 24, 2022
View reviewed changes

dapengzhang0 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

punya merged commit 2e90f49 into census-instrumentation:master Jan 24, 2022

mackenziestarr commented Jan 24, 2022

Copy link
Copy Markdown
Contributor Author

thanks for updating those @punya, appreciate it 😄

gcf-merge-on-green Bot referenced this pull request in googleapis/java-spanner Feb 2, 2022
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 [https://github.com/census-instrumentation/opencensus-java/pull/2091](https://togithub.com/census-instrumentation/opencensus-java/pull/2091)
-   implement gRPC client retry stats measures and views by [@&#8203;mackenziestarr](https://togithub.com/mackenziestarr) in [https://github.com/census-instrumentation/opencensus-java/pull/2084](https://togithub.com/census-instrumentation/opencensus-java/pull/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 referenced this pull request in googleapis/google-http-java-client Feb 9, 2022
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 [https://github.com/census-instrumentation/opencensus-java/pull/2091](https://togithub.com/census-instrumentation/opencensus-java/pull/2091)
-   implement gRPC client retry stats measures and views by [@&#8203;mackenziestarr](https://togithub.com/mackenziestarr) in [https://github.com/census-instrumentation/opencensus-java/pull/2084](https://togithub.com/census-instrumentation/opencensus-java/pull/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 mentioned this pull request Feb 11, 2022
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

rghetia Awaiting requested review from rghetia
songy23 Awaiting requested review from songy23
jsuereth Awaiting requested review from jsuereth
3 more reviewers
asafdav2 asafdav2 left review comments
punya punya approved these changes
dapengzhang0 dapengzhang0 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

punya

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL