-
Notifications
You must be signed in to change notification settings - Fork 1
PowerHandler with Prometheus #5
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
Conversation
lambda_powertools/prometheus.py
Outdated
|
||
modulename = 'prometheus_client' | ||
if modulename in sys.modules: | ||
raise Exception("prometheus_client already imported, lambda_powertools.prometheus must be imported before") |
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.
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 possible, describe why you need this. So that I understand what you've been hitting and see if it can be handled differently in a future iteration.
btw, if it's only related to the environment variable that I see below, the module can be reloaded
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.
Because the global REGISTRY
is instantiated at the first prometheus_client import, and the PROMETHEUS_DISABLE_CREATED_SERIES
should be set before the first import. The problem is described here (prometheus/client_python#933) and I see now that they just released a configuration for that (https://github.com/prometheus/client_python/releases/tag/v0.18.0). So maybe I can remove that shit 😅
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.
Code looks good to me. We should release it, and then attach it to the lambda observability example! 🚀
This reverts commit 08740f7.
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.
it looks alien to me. so i trust muro's and lele's eyes that it does what it's supposed to.
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.
I'm not familiar with prom client, so even though the code looks a bit hackish, I didn't comment specifics. Just left comments to improve the code
lambda_powertools/prometheus.py
Outdated
|
||
modulename = 'prometheus_client' | ||
if modulename in sys.modules: | ||
raise Exception("prometheus_client already imported, lambda_powertools.prometheus must be imported before") |
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 possible, describe why you need this. So that I understand what you've been hitting and see if it can be handled differently in a future iteration.
btw, if it's only related to the environment variable that I see below, the module can be reloaded
lambda_powertools/prometheus.py
Outdated
|
||
|
||
def reset(): | ||
for collector in list(prometheus_client.REGISTRY._collector_to_names.keys()): |
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.
You don't need to create a list to iterate over the keys. You could simply use:
for collector in prometheus_client.REGISTRY._collector_to_names.keys():
Or simply do:
for collector in prometheus_client.REGISTRY._collector_to_names:
lambda_powertools/prometheus.py
Outdated
|
||
def filter_metrics(m): | ||
# Other metric types are not supported so far | ||
if not m._type == "counter" and not m._type == "histogram": |
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.
Why not:
if m._type != "counter" and m._type != "histogram":
or:
if m._type not in ("counter", "histogram"):
lambda_powertools/prometheus.py
Outdated
def get_metrics(): | ||
new_registry = CollectorRegistry(auto_describe=True) | ||
|
||
for collector in list(filter(filter_metrics, prometheus_client.REGISTRY._collector_to_names.keys())): |
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.
Same comment I left in the reset
function
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.
👍
Automatic support for Prometheus metrics via PROMLOGs in the power_handler.
Based on what done here https://github.com/spreaker/lambda-observability/pull/63 and here https://github.com/spreaker/node-lambda-powertools/blob/master/src/prometheus.js
Related thread on Slack