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

fix: refine example grafana dashboard #3457

Merged
merged 11 commits into from
Mar 26, 2021
Merged

Conversation

Yiyiyimu
Copy link
Member

What this PR does / why we need it:

Ref #3402

  • update grafana to 'for external use' version
  • add screenshot of new panels

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
@@ -120,6 +120,10 @@ Or you can goto [Grafana official](https://grafana.com/grafana/dashboards/11719)

![](../../doc/images/plugin/grafana_3.png)

![](../../doc/images/plugin/grafana_4.png)
Copy link
Member

@membphis membphis Jan 29, 2021

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

There are currently two problems for histogram:

  1. The legends have leading for trailing zeros (reason) so it looks messy
  2. There are two many buckets out there. We don't need buckets like "30,40,50,60,70,80,90,100,200,300,400,500". After reduce buckets, it somewhat looks better
    image

Or we could use 95-percentile as @tokers said. I'm not so familiar with monitor APISIX, so what do you think about that

Copy link
Member Author

@Yiyiyimu Yiyiyimu Jan 31, 2021

Choose a reason for hiding this comment

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

For etcd-reachable and nginx metric errors, the comments of what these two metrics do are explained on the metrics page alongside with the data. And for how I implement it

  • etcd-reachable: when etcd is reachable, the metric would be 1 and the graph would be green. While it is disconnected, the metric turns to 0 and the panel would be red.
  • nginx metric errors: it measures how many errors of nginx-lua-prometheus. I also leave it to only show the current error number, since if it is larger than 0, it should be taken care of. And also if it is larger than 0, it would turn red.

Copy link
Member Author

Choose a reason for hiding this comment

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

would fix leading for trailing zeros in knyar/nginx-lua-prometheus#119

Copy link
Member Author

@Yiyiyimu Yiyiyimu Feb 7, 2021

Choose a reason for hiding this comment

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

@membphis @tokers PTAL

Updated to newer version of nginx-lua-prometheus, and it would looks like
image

@tokers
Copy link
Contributor

tokers commented Jan 30, 2021

I think the HTTP requests latency can be updated to only show the 95th percentile (or 90th?), it's not easy to understand now.

@Yiyiyimu
Copy link
Member Author

Yiyiyimu commented Feb 1, 2021

waiting for knyar/nginx-lua-prometheus#119 got merged


PR got merged, waiting for new release


Applied the new release

@Yiyiyimu Yiyiyimu changed the title chore: update grafana to 'for external use' version fix: refine example grafana dashboard Feb 1, 2021
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
@membphis
Copy link
Member

I think the HTTP requests latency can be updated to only show the 95th percentile (or 90th?), it's not easy to understand now.

agree +1

Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Signed-off-by: yiyiyimu <wosoyoung@gmail.com>
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

LGTM, and I think we can update this part too.

image

@starsz starsz merged commit 83931ba into apache:master Mar 26, 2021
@Yiyiyimu
Copy link
Member Author

Yiyiyimu commented Mar 26, 2021

LGTM, and I think we can update this part too.

image

Hi @LiteSun could you offer some help here? I'm not familiar with frontend layout 😬
Fixed in #3993

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants