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(server): add runner metrics; refactoring batch size metrics #2977

Merged
merged 10 commits into from Sep 16, 2022

Conversation

bojiang
Copy link
Member

@bojiang bojiang commented Sep 9, 2022

What does this PR address?

Fixes https://github.com/bentoml/Kitchen/issues/40

Before submitting:

Who can help review?

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

@bojiang bojiang requested review from ssheng, parano and a team as code owners September 9, 2022 03:29
@bojiang bojiang requested review from jjmachan and removed request for a team September 9, 2022 03:29
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.

I think we should also have backward compatibility for older configuration where api_server.metrics field under yaml.

@codecov
Copy link

codecov bot commented Sep 9, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@616787a). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head cabaf00 differs from pull request most recent head b4ab719. Consider uploading reports for the commit b4ab719 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2977   +/-   ##
=======================================
  Coverage        ?   70.61%           
=======================================
  Files           ?      104           
  Lines           ?     9570           
  Branches        ?        0           
=======================================
  Hits            ?     6758           
  Misses          ?     2812           
  Partials        ?        0           

@aarnphm
Copy link
Member

aarnphm commented Sep 13, 2022

@bojiang can you also add a check for the prefix in our tracking code here?

and metric.name.startswith("BENTOML_")

@bojiang
Copy link
Member Author

bojiang commented Sep 13, 2022

@aarnphm There's no breaking change in this PR. Those metrics are still there.

bentoml/_internal/server/instruments.py Show resolved Hide resolved
START_TIME_VAR.set(default_timer())

async def wrapped_send(message: "ext.ASGIMessage") -> None:
if message["type"] == "http.response.start":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we register time after the last byte of body is sent like in access.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

never thought about that

bentoml/_internal/server/instruments.py Show resolved Hide resolved
Comment on lines +265 to +266
"--prometheus-dir",
prometheus_dir,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed to make runner metrics work?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. Runners might be multi-processing, too

@@ -96,25 +134,158 @@ async def wrapped_send(message: "ext.ASGIMessage") -> None:
status_code = message["status"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's change the behavior for api_server, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

ooops

ssheng
ssheng previously approved these changes Sep 15, 2022
@bojiang bojiang enabled auto-merge (squash) September 15, 2022 10:47
@bojiang bojiang merged commit 532f5e1 into bentoml:main Sep 16, 2022
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.

None yet

3 participants