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

grafana: Add gRPC Received commands rate panel #8921

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

okJiang
Copy link
Member

@okJiang okJiang commented Dec 16, 2024

What problem does this PR solve?

Issue Number: Close #8920

What is changed and how does it work?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code
image

Release note

None.

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. labels Dec 16, 2024
Copy link
Contributor

ti-chi-bot bot commented Dec 16, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign huachaohuang for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@okJiang okJiang changed the title grafana: Add gRPC Received commands rate grafana: Add gRPC Received commands rate panel Dec 16, 2024
@ti-chi-bot ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 16, 2024
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.31%. Comparing base (0b757ce) to head (9888398).
Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8921      +/-   ##
==========================================
+ Coverage   76.20%   76.31%   +0.11%     
==========================================
  Files         461      462       +1     
  Lines       70448    70447       -1     
==========================================
+ Hits        53686    53765      +79     
+ Misses      13402    13336      -66     
+ Partials     3360     3346      -14     
Flag Coverage Δ
unittests 76.31% <ø> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

"spaceLength": 10,
"targets": [
{
"expr": "sum(rate(grpc_server_started_total{k8s_cluster=\"$k8s_cluster\", tidb_cluster=\"$tidb_cluster\", instance=~\"$instance\"}[1m])) by (instance, grpc_method)",
Copy link
Member

Choose a reason for hiding this comment

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

How about also adding the error rate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it. But btw, it can't catch the error hide in the header, like this

Header: wrapErrorToHeader(pdpb.ErrorType_REGION_NOT_FOUND,

Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment or raise an issue for further improvement.

Signed-off-by: okJiang <[email protected]>
"spaceLength": 10,
"targets": [
{
"expr": "sum(rate(grpc_server_handled_total{k8s_cluster=\"$k8s_cluster\", tidb_cluster=\"$tidb_cluster\", instance=~\"$instance\", grpc_type=\"unary\", grpc_code!=\"OK\"}[1m])) by (grpc_method)",
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding one expr as unary total about ignoring grpc_code?

Copy link
Member Author

@okJiang okJiang Dec 23, 2024

Choose a reason for hiding this comment

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

grpc_server_handled_total only records the count after handling. If we want to record the receive msg count, we can use grpc_server_msg_received_total, but it does not record grpc_code label. So we can't record the error rate of the stream, no grpc native metrics we can use. https://github.com/grpc-ecosystem/go-grpc-middleware/blob/66bb7d8818ff22b450b82111a1260ca6dfe3486c/providers/prometheus/server_metrics.go#L27-L53

@okJiang
Copy link
Member Author

okJiang commented Dec 23, 2024

/retest

@okJiang
Copy link
Member Author

okJiang commented Dec 26, 2024

/retest

@okJiang
Copy link
Member Author

okJiang commented Dec 26, 2024

ping @rleungx @bufferflies

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has signed the dco. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add gRPC received command rate
3 participants