-
Notifications
You must be signed in to change notification settings - Fork 908
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
Metrics: merge different metrics into series + labels #3309
Comments
/assign Agree. Yes, we can have a general updated approach for Prometheus metrics. Something like if any "metric" can split out into 5+ sub-metrics or similar we follow the #3272 approach. Else we keep metrics in 1:1 sync with the JSON rule output. It would also apply to the new per CPU counters Andrea added. @sgaist would you have additional thoughts? @Issif we previously concluded that it would be fine to keep the memory related metrics as separate metrics. Do we want to stick to that or is there a desire to re-discuss and change that as well given we now introduce more breaking changes? @leogr updated info: This would mean a follow up breaking change for Prometheus metrics, but not the rule output metrics. |
for the memory it's ok to keep it has it, we're talking about 3 metrics, not dozens like for the drops or the rule counters. |
Imho that's an incubating feature thus it is somewhat expect to rapidly change to the desired final design, then we'll promote it to stable. Btw thanks for tackling this once again, it's been a pleasure to work with you and @sgaist on this :) |
Agree
Also @FedeDP agreed. Just in case we need another round of tweaks 🙃 fingers crossed the metrics framework soon stabilizes. |
Update: As discussed in yesterday's maintainer meeting, the enter/exit directions should be reported as a label. For example:
Note that we must also review all other metrics to ensure they are consistent with this pattern. Final note: introducing breaking change at this stage shouldn't be an issue; the metrics feature will still be kept as an "incubating" maturity level for 0.39 |
PR is up, please help check the initial test data, ty! @leogr perhaps
|
cc @Issif PTAL 🙏 |
Seems good to me 👍 |
Another fix / breaking change is that I removed the double falco here All Prometheus kernel counters had the wrong subsystem, it is "scap", not "falco" ... so all these metric names are broken Also the label raw_name is now removed, not a breaking change necessarily @leogr maybe an easy message for Falco 0.39.0 could be: Prometheus metrics names should be considered broken compared to previous releases. On that note do we need / want any additional changes in names, also @Issif ? After we merge this I can stage the website update PR. |
See also #3324 |
Describe the bug
I started to work on the Grafana dashboard for the falco metrics, and saw something we should change, if possible before the new release.
It concerns the
drops_enter/exit
metrics, but I think others are concerned (likethe _scap_
but I'm not sure if they measure the same things in different context or not)For the
drops
we have this:it should be something like this:
by this way, we can run promql queries like this one:
and get all drops in a single graph (a pie chart for example), without running 1 query by metric
How to reproduce it
call the /metrics endpoint to get the available series
Expected behaviour
Screenshots
Environment
Additional context
The text was updated successfully, but these errors were encountered: