fix: prometheus multproc dir permissions error#118
fix: prometheus multproc dir permissions error#118matthewelwell wants to merge 19 commits intomainfrom
Conversation
matthewelwell
requested review from
gagantrivedi
and removed request for
a team
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (92.85%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
- Coverage 94.63% 94.59% -0.05%
==========================================
Files 77 78 +1
Lines 2459 2477 +18
==========================================
+ Hits 2327 2343 +16
- Misses 132 134 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
matthewelwell
changed the title
| } | ||
|
|
||
| timer.labels(**labels) # type: ignore[no-untyped-call] | ||
| timer.labels(**labels) |
There was a problem hiding this comment.
I don't really know why mypy pulled me up on this in this PR given that I've not touched this file, or any related code...
There was a problem hiding this comment.
I don't love that this code can't live in utils itself, but see comment in __init__.py for reasoning.
There was a problem hiding this comment.
Perhaps the more logical answer here would be to move Histogram out of utils and keep this function in there?
| from common.prometheus.utils import Histogram | ||
| from typing import Any | ||
|
|
||
| __all__ = ("Histogram",) | ||
| _utils = ("Histogram",) | ||
|
|
||
|
|
||
| def __getattr__(name: str) -> Any: | ||
| """ | ||
| Since utils imports django.conf.settings, we lazy load any objects that | ||
| we want to import to prevent django.core.exceptions.ImproperlyConfigured | ||
| due to settings not being configured. | ||
| """ | ||
| if name in _utils: | ||
| from common.prometheus import utils | ||
|
|
||
| return getattr(utils, name) | ||
| raise AttributeError(name) |
There was a problem hiding this comment.
This is pretty ugly, but might actually save us pain in the future, as well as fixing the pain I experienced. There are a couple of alternatives:
- In
utils.py, use a factory instead of subclassingHistogram, and perform a local import ofdjango.conf.settings - Take a slightly different approach and just split up the logical pieces of
prepare_prom_multiproc_dirinto generic functions where necessary likeclear_sub_dirs(dir: str), and just call it step by step inensure_cli_env
|
Superseded by #120 |
2 participants
See CI failures / conversations in this PR in the core repository for more information on why this is necessary.
Further context can also be found here.