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(prometheus): ensure metrics are lazily loaded #3089

Merged
merged 18 commits into from Oct 28, 2022

Conversation

aarnphm
Copy link
Member

@aarnphm aarnphm commented Oct 11, 2022

This PR address the issue inside the container, where if the user service uses bentoml.metrics, the service is imported before PROMETHEUS_MULTIPROC_DIR is created.

Signed-off-by: Aaron Pham 29749331+aarnphm@users.noreply.github.com

@aarnphm aarnphm requested a review from a team as a code owner October 11, 2022 20:57
@aarnphm aarnphm requested review from larme and removed request for a team October 11, 2022 20:57
@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #3089 (75fce2b) into main (d564519) will decrease coverage by 0.00%.
The diff coverage is 8.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3089      +/-   ##
==========================================
- Coverage   21.73%   21.73%   -0.01%     
==========================================
  Files         101      101              
  Lines        9709     9710       +1     
  Branches     1683     1685       +2     
==========================================
  Hits         2110     2110              
- Misses       7551     7552       +1     
  Partials       48       48              
Impacted Files Coverage Δ
...c/bentoml/_internal/runner/runner_handle/remote.py 0.00% <0.00%> (ø)
src/bentoml/_internal/server/metrics/prometheus.py 0.00% <0.00%> (ø)
src/bentoml/_internal/utils/__init__.py 25.09% <14.28%> (-0.47%) ⬇️

@aarnphm aarnphm force-pushed the chore/metrics branch 3 times, most recently from e10e408 to 6555340 Compare October 19, 2022 07:08
@aarnphm aarnphm enabled auto-merge (squash) October 19, 2022 07:09
@bojiang bojiang self-requested a review October 20, 2022 02:13
@bojiang
Copy link
Member

bojiang commented Oct 20, 2022

@aarnphm Why do we need this?

@aarnphm
Copy link
Member Author

aarnphm commented Oct 20, 2022

When users use bentoml.metrics inside service.py, this would currently fail, since the multiproc dir is not yet initialized (inside container)

@aarnphm aarnphm force-pushed the chore/metrics branch 6 times, most recently from d95cca4 to d565380 Compare October 26, 2022 07:17
@aarnphm aarnphm changed the title fix(prometheus): ensure the multiproc dir is created before importing service fix(prometheus): ensure metrics are lazily loaded Oct 26, 2022
src/bentoml/metrics.py Outdated Show resolved Hide resolved
src/bentoml/metrics.py Outdated Show resolved Hide resolved
@bojiang
Copy link
Member

bojiang commented Oct 27, 2022

@aarnphm The test is still failling

@bojiang
Copy link
Member

bojiang commented Oct 27, 2022

It seems the e2e tests are skipped. Let's continue the review when it's enabled

@aarnphm aarnphm force-pushed the chore/metrics branch 2 times, most recently from 5da119a to 484d32e Compare October 28, 2022 04:03
@aarnphm aarnphm force-pushed the chore/metrics branch 2 times, most recently from aaba4c2 to ee4ff3c Compare October 28, 2022 13:43
pyproject.toml Outdated Show resolved Hide resolved
src/bentoml/_internal/server/metrics/prometheus.py Outdated Show resolved Hide resolved
src/bentoml/_internal/utils/__init__.py Outdated Show resolved Hide resolved
aarnphm and others added 18 commits October 28, 2022 14:55
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Co-authored-by: Sauyon Lee <2347889+sauyon@users.noreply.github.com>
Co-authored-by: Sauyon Lee <2347889+sauyon@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
@aarnphm aarnphm merged commit 25c4b12 into bentoml:main Oct 28, 2022
@aarnphm aarnphm deleted the chore/metrics branch October 28, 2022 22:42
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