-
Notifications
You must be signed in to change notification settings - Fork 7
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(backend): Demonstrate metrics #1488
base: dev
Are you sure you want to change the base?
Conversation
Signed-off-by: Sylvain Leclerc <[email protected]>
Features
ContributorsCommit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
antarest/core/config.py
Outdated
@staticmethod | ||
def from_dict(data: JSON) -> "PrometheusConfig": | ||
return PrometheusConfig(multiprocess=bool(data["multiprocess"])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pythonic way to implement multiple constructors is to use @classmethod
. See this article about "Providing Multiple Constructors in Your Python Classes".
@classmethod
def from_dict(cls, data: JSON) -> "PrometheusConfig":
return cls(multiprocess=bool(data["multiprocess"]))
antarest/core/config.py
Outdated
@staticmethod | ||
def from_dict(data: JSON) -> "MetricsConfig": | ||
return MetricsConfig( | ||
prometheus=PrometheusConfig.from_dict(data["prometheus"]) | ||
if "prometheus" in data | ||
else None | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use @classmethod
instead
antarest/core/exceptions.py
Outdated
def __init__(self, message: str) -> None: | ||
super().__init__(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not really useful unless you add some information, for instance, you can use the message template here (I mean "Environment variable {_PROMETHEUS_MULTIPROCESS_ENV_VAR} must be defined for use of prometheus in a multiprocess environment")
logger = logging.getLogger(__name__) | ||
|
||
|
||
_PROMETHEUS_MULTIPROCESS_ENV_VAR = "PROMETHEUS_MULTIPROC_DIR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a short comment to explain the purpose of this environment variable, for instance:
# The `PROMETHEUS_MULTIPROC_DIR` environment variable is used by
# the Python Prometheus client library to configure process-level metrics
# when running in a multi-process environment.
antarest/core/metrics.py
Outdated
if _PROMETHEUS_MULTIPROCESS_ENV_VAR not in os.environ: | ||
raise ConfigurationError( | ||
f"Environment variable {_PROMETHEUS_MULTIPROCESS_ENV_VAR} must be defined for use of prometheus in a multiprocess environment" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We check that this environment variable exists but it is not used directly in the code (but only in the client). So, a little explanation is needed.
Moreover, we should also check that it is a directory that exists (with the right permissions).
@@ -8,7 +8,7 @@ checksumdir~=1.2.0 | |||
click~=8.0.3 | |||
contextvars~=2.4 | |||
fastapi-jwt-auth~=0.5.0 | |||
fastapi[all]~=0.73.0 | |||
fastapi[all]~=0.74.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: A version upgrade would be welcome to take advantage of the latest developments that simplify the update of the Swagger API documentation. But, we need to analyse the impacts...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! This was the minimum upgrade to get the "route" information from the request
""" | ||
Put the worker_id into an env variable for further use within the app. | ||
""" | ||
os.environ["APP_WORKER_ID"] = str(worker.age) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had a choice, I would prefer GUNICORN_WORKER_ID
for that.
Why do you use worker.age
instead of worker.id
?
worker.age
is a number that indicates how many requests the worker process has handled since it was started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understood, worker.age
is a counter incremented every time gunicorn creates a new worker (for ex. when one dies).
Using the age here allows to have an almost stable set of worker IDs between application runs (after a restart of the app, for example, we sill have IDs in the range [1-8] for 8 workers), to have mostly stable metrics labels.
I don't see any worker.id
, we have the PID as an alternative, but it's not stable at all between runs.
Signed-off-by: Sylvain Leclerc <[email protected]>
Signed-off-by: Sylvain Leclerc <[email protected]>
Signed-off-by: Sylvain Leclerc <[email protected]>
@sylvlecl : do you know about that : https://autometrics.dev/ |
Description
This is a draft for defining metrics and exposing them to a prometheus instance, in order to improve the application observability.
The only metrics that are defined fro now are metrics about HTTP requests (count, duration).
Some technical details:
In the future, we could add metrics about various things: DB sessions, tasks durations, ...
In the long run, using opentelemetry could be an alternative.
As a very basic illustration of grah in grafana: