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

feat: update variables in metric names as labels #2969

Closed
wants to merge 2 commits into from

Conversation

ssheng
Copy link
Collaborator

@ssheng ssheng commented Sep 6, 2022

What does this PR address?

BentoML metrics name used to encode service names, runner names, and worker index etc, into the metric names. This practice however is not friendly to setting up dashboards in Prometheus and Grafana. This PR moves all variables parts in the metric names as labels and hardcode bentoml as the prefix. This PR also maintains backward compatibility by continue to produce metrics in the legacy format.

Legacy metrics:

BENTOML_iris_classifier_request_total
BENTOML_iris_classifier_request_duration_seconds_sum
BENTOML_iris_classifier_request_duration_seconds_bucket
BENTOML_iris_classifier_request_duration_seconds_count

New metrics:

bentoml_request_total
bentoml_request_duration_seconds_sum
bentoml_request_duration_seconds_bucket
bentoml_request_duration_seconds_count

Both sets of the metrics will be supported until we can phase out the legacy one.

Fixes #2963

Before submitting:

Who can help review?

Feel free to tag members/contributors who can help review your PR.

@ssheng ssheng requested a review from a team as a code owner September 6, 2022 07:03
@ssheng ssheng requested review from parano and bojiang and removed request for a team September 6, 2022 07:03
@codecov
Copy link

codecov bot commented Sep 6, 2022

Codecov Report

Merging #2969 (42c188b) into main (43c4eae) will decrease coverage by 0.63%.
The diff coverage is 72.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2969      +/-   ##
==========================================
- Coverage   70.60%   69.96%   -0.64%     
==========================================
  Files         104      104              
  Lines        9538     9683     +145     
==========================================
+ Hits         6734     6775      +41     
- Misses       2804     2908     +104     
Impacted Files Coverage Δ
bentoml/_internal/server/metrics/prometheus.py 78.94% <ø> (-9.12%) ⬇️
bentoml/_internal/marshal/dispatcher.py 65.58% <70.37%> (-18.85%) ⬇️
bentoml/_internal/server/instruments.py 63.29% <72.22%> (-36.71%) ⬇️
bentoml/_internal/configuration/containers.py 75.98% <100.00%> (-2.53%) ⬇️
bentoml/_internal/utils/metrics.py 96.66% <100.00%> (ø)

Copy link
Member

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

Where do we hardcode the namespace for metrics?
https://prometheus.io/docs/practices/naming/#metric-names

@aarnphm aarnphm closed this Sep 18, 2022
@aarnphm
Copy link
Member

aarnphm commented Sep 18, 2022

addressed in #3008

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.

feature: move model name & bentoml from prefix to label in metrics
2 participants