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

[KYUUBI #5834] Add Grafana dashboard template #5147

Closed
wants to merge 8 commits into from

Conversation

zhaohehuhu
Copy link
Contributor

@zhaohehuhu zhaohehuhu commented Aug 9, 2023

Why are the changes needed?

This PR adds a basic Grafana Dashboard template, also updates the metrics docs to guide users to use Prometheus and Grafana to monitor the Kyuubi server.

The Grafana Dashboard template is exported from the Grafana OSS v11.4.0

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

image
  • Run test locally before make a pull request

@zhaohehuhu
Copy link
Contributor Author

zhaohehuhu commented Aug 9, 2023

plz help review @pan3793 @zwangsheng . Thanks all.😄

@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (b265ccb) to head (f6fc2d7).
Report is 2 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #5147   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         687     687           
  Lines       42463   42463           
  Branches     5796    5796           
======================================
  Misses      42463   42463           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bowenliang123
Copy link
Contributor

bowenliang123 commented Aug 9, 2023

Could you append a full screenshot to the desciption for demonstrating the dashboard?
And any further detailed explanation for datasource settings, metric group and etc.

@zwangsheng
Copy link
Contributor

  1. 截屏2023-08-10 09 38 07
    We should set max as 100.

  2. 截屏2023-08-10 09 38 15
    Don't know why threshold is needed here, let's remove those operation-num thresholds.

@zhaohehuhu
Copy link
Contributor Author

zhaohehuhu commented Aug 10, 2023

Could you append a full screenshot to the desciption for demonstrating the dashboard? And any further detailed explanation for datasource settings, metric group and etc.

Added screenshots. Thanks.

@zhaohehuhu
Copy link
Contributor Author

zhaohehuhu commented Aug 10, 2023

Yup. We can remove the threshold if it's not required. @zwangsheng

@pan3793 pan3793 added this to the v1.8.0 milestone Sep 1, 2023
@zhaohehuhu
Copy link
Contributor Author

plz review it @pan3793

# under the grafana folder.
# 1. docker build --build-arg PROMETHEUS_URL_ARG="" -t grafana:kyuubi -f Dockerfile .
# Options:
# --build-arg the url to access promethues datasource
Copy link
Member

Choose a reason for hiding this comment

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

what's the value should be like? could you please add an example in the description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it.

# limitations under the License.
#

# Usage:
Copy link
Member

Choose a reason for hiding this comment

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

please create a REAMDE.md in the same folder, and move this guidance to README.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

appreciate it. I already provided REAMDE.md to guide users.

@pan3793
Copy link
Member

pan3793 commented Sep 22, 2023

Currently, the default value of kyuubi.metrics.reporters is JSON, as PROMETHEUS becomes more popular today, how do you think changing it to PROMETHEUS in 1.8?

@yaooqinn @bowenliang123

ENV PROMETHEUS_URL=$PROMETHEUS_URL_ARG
COPY ./dashboard.yml /etc/grafana/provisioning/dashboards/
COPY ./datasource.yml /etc/grafana/provisioning/datasources/
COPY ./*.json /var/lib/grafana/dashboards/
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add empty line at end of file.

editable: true
allowUiUpdates: true
options:
path: /var/lib/grafana/dashboards/
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

access: proxy
url: ${PROMETHEUS_URL}
isDefault: true
editable: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@pan3793 pan3793 modified the milestones: v1.8.0, v1.8.1 Nov 6, 2023
@pan3793 pan3793 mentioned this pull request Dec 8, 2023
3 tasks
@pan3793 pan3793 changed the title [Improvement]Add Grafana dashboard template [KYUUBI #5834] Add Grafana dashboard template Dec 8, 2023
@zhaohehuhu
Copy link
Contributor Author

@pan3793 plz help review it

@yaooqinn
Copy link
Member

yaooqinn commented Jan 5, 2024

Currently, the default value of kyuubi.metrics.reporters is JSON, as PROMETHEUS becomes more popular today, how do you think changing it to PROMETHEUS in 1.8?

@yaooqinn @bowenliang123

I'm OK with this, but it seems that we'd better to raise a discussion thread in dev

@bowenliang123
Copy link
Contributor

Currently, the default value of kyuubi.metrics.reporters is JSON, as PROMETHEUS becomes more popular today, how do you think changing it to PROMETHEUS in 1.8?
@yaooqinn @bowenliang123

I'm OK with this, but it seems that we'd better to raise a discussion thread in dev

The default metrics reporter has been set to Prometheus in #5344.

@yaooqinn
Copy link
Member

yaooqinn commented Jan 5, 2024

Thank you @bowenliang123

@sudohainguyen
Copy link
Contributor

hey folks, any blockers on this 🤔 ?

@zhaohehuhu
Copy link
Contributor Author

hey folks, any blockers on this 🤔 ?

It still needs some time to be reviewed.

@hakeedra
Copy link

Hi, @zhaohehuhu some metrics do not include instance filtering, such as the expression in 'Thread Status'. When testing with Kyuubi version '1.9.1', metric names may depending on the Garbage Collector. For example, 'kyuubi_memory_usage_pools_PS_Eden_Space_used' may appear as 'kyuubi_memory_usage_pools_Eden_Space_used'.

@zhaohehuhu
Copy link
Contributor Author

zhaohehuhu commented Nov 16, 2024

Hi, @zhaohehuhu some metrics do not include instance filtering, such as the expression in 'Thread Status'. When testing with Kyuubi version '1.9.1', metric names may depending on the Garbage Collector. For example, 'kyuubi_memory_usage_pools_PS_Eden_Space_used' may appear as 'kyuubi_memory_usage_pools_Eden_Space_used'.

Thanks. I'll update it.

Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

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

I'd like to merge this PR and polish the dashboard in follow-ups.

@pan3793
Copy link
Member

pan3793 commented Dec 24, 2024

Thanks, merging to master

@pan3793 pan3793 closed this in 7e8275b Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants