-
-
Notifications
You must be signed in to change notification settings - Fork 283
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
Add extra labels to prometheus report (warn and error level only) #68
Conversation
Basic details about issues. Can be largely improved just a POC
@sky-philipalmeida Thank you for the PR and thoughts!! One of the thing I love most about prometheus is it's all about that one thing ie gather metrics and not trying or pretending to be for anything else. IMHO this edges a bit off this essence as the labels are only there to support queries for series. I feel that Popeye prom output should all be about gathering signals and not shoehorn sanitization details. ie labels should be used for dimensionality and not kitchen drawers. Perhaps storing in a label a sanitizer report ID might be a better approach here? That said, I am open for debate here of course, but these are my initial thoughts. |
@derailed I understand your pov. So your are thinking, eg., to use the save option and later on alertmanager expose the saved file using a url by report id? That makes sense for sure and it is the proper way I think... ...but, there are some caveats afaik, I just checked and the current save options do not play well with HTML browser view, also prometheus output format does not support file save. So if this is the case afaik there should be a new HTML compatible report and support for the -s option on prometheus output format (that would also save to HTML if -s enabled). Sincerely I think both approaches are valid. Have a flag that the user could force a label with the issue list (basically my initial approach), this would make possible queries to specific namespaces, or only to specifc error codes which also makes sense. If you are ok with this I have time to refactor in order to overcome the current Codesense as this is a quick win for my goal. What do you think? |
@sky-philipalmeida Thank you so much for voicing your thoughts! I think I agree with your approach so let me make sure we're on the same page. Currently a user can generate and push prom metrics using the following command: popeye -o prometheus --pushgateway-address=xxx We want to store the associated sanitizer report location as a label along with the prom metrics. popeye -o prometheus,json --pushgateway-address=xxx --save So this command would allow the user to generate multiple report outputs and save them to disk and in the case the output is prometheus we would add a new label to the metrics with the location of the sanitizer report. This does also allow for generating different outputs in a single command no matter if prom is specified or not which I think could be useful. We generate the sanitization once and output to different formats.
Would this satisfy your use cases? or can you think of a better way to achieve this? Thank you Phil!! |
5b51fe2
to
12aece9
Compare
12aece9
to
4fc1c4b
Compare
@derailed enabling multiple output formats will require careful refactor and maybe should be addressed in another feature request IMHO. The current proposal enables:
Having the path on the target label would need maybe a webserver exposing the files in order to be usefull in Alertmanager links but that is just an ideia. current implementation status:
|
@sky-philipalmeida Thank you for review this! I think you are right we can tackle multiple reports in a separate push. As for the report location, I think we might need a different option. The report url could be provided in many shapes ie via http or others. So I am thinking perhaps --save does not quiet fit the bill. Looping in @guusvw here) are he owns the PR for s3 support. NOTE: Not keen on the --out-details what happens if you reports are generating 10k+ issues per runs? I am kind of burnt this week but we need to think about --save being a flag to indicate how we generate the report url. ie file/http/s3 etc.. So I think perhaps --save file|s3|etc... might be more inline with what we're trying to achieve here?? popeye -o prometheus --pushgateway-address http://localhost:9091 --save=s3 -s3bucket=xxx |
I really like the idea of defining a "save storage" via |
@@ -226,6 +234,7 @@ func (p *Popeye) ensureOutput() error { | |||
ext := "txt" | |||
switch *p.flags.Output { | |||
case "json": | |||
case "prometheus": |
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.
case "prometheus": | |
case "json", "prometheus": |
Basic details about issues for prometheus report.
Afaik it can be largely improved.
Capabilities:
Atm the final result on the Prometheus POV would be (example for service):
$popeye -o prometheus --pushgateway-address http://localhost:9091 -l error -n testns --out-detail verbose
popeye_sanitizer_reports_count{cluster="testcluster",issues="testns/kafka [POP-1100] No pods match service selector,testns/kafka [POP-1105] No associated endpoints",job="popeye",level="error",namespace="testns/kafka",resource="service"} | 1