ansible icon indicating copy to clipboard operation
ansible copied to clipboard

enhancement(prometheus): support prometheus2 .yml rule file format

Open dpavle opened this issue 1 year ago • 7 comments

dpavle avatar Apr 15 '24 11:04 dpavle

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and the docs are now incorporated into main: https://prometheus-community.github.io/ansible/branch/main

github-actions[bot] avatar Apr 15 '24 11:04 github-actions[bot]

Please also update the updated docs.

SuperQ avatar Apr 15 '24 14:04 SuperQ

There are some other references to .rules files in the role. Maybe worth cleaning up more.

SuperQ avatar Apr 16 '24 20:04 SuperQ

If we want to support both .rules and .yml as proposed by this PR then both should be listed in the docs and argument specs.

But if we want to drop .rules then that should be done clearly with a warning in the preflight.

Also .yaml should be supported since both.yml and .yaml are valid yaml file extensions.

I'm for removing the old .rules format. It's essentially been deprecated since 2017 and it doesn't work in any recent version of Prometheus as far as I'm aware.

As for the warning in the preflight, sure, that could be done. I'm just not sure how exactly, do we fail if any .rules files are found in /rules?

And yes, .yaml should also be supported.

dpavle avatar Apr 17 '24 10:04 dpavle

We probably want to keep the *.rules handling, at least from a configuration loading perspective, around in order to maintain backwards compatibility. Otherwise existing deployments that are pushing yaml to files named .rules will break.

SuperQ avatar Apr 17 '24 12:04 SuperQ

We probably want to keep the *.rules handling, at least from a configuration loading perspective, around in order to maintain backwards compatibility. Otherwise existing deployments that are pushing yaml to files named .rules will break.

That's why I suggested a error in the preflight, but we could also make that a warning for now and then drop *.rules support in the future. Or we could handle it automatically by adding a task that renames .rules to .yml

gardar avatar Apr 17 '24 13:04 gardar

We probably want to keep the *.rules handling, at least from a configuration loading perspective, around in order to maintain backwards compatibility. Otherwise existing deployments that are pushing yaml to files named .rules will break.

That's why I suggested a error in the preflight, but we could also make that a warning for now and then drop *.rules support in the future. Or we could handle it automatically by adding a task that renames .rules to .yml

A warning works. If there are any YAML formatted files with the .rules extension there will be a warning message in the preflight and the rule validation will pass normally. On the other hand, if there are any rules in the old .rules format, the warning message will show in the preflight and promtool check validation will catch them and fail in a later step.

dpavle avatar Apr 18 '24 10:04 dpavle

@dpavle any chance you could refactor this so that we can get it merged?

gardar avatar Oct 30 '24 16:10 gardar

@dpavle any chance you could refactor this so that we can get it merged?

sure, I'll take a look at it

dpavle avatar Oct 30 '24 17:10 dpavle

@SuperQ any outstanding issues or are we good to go?

gardar avatar Oct 31 '24 16:10 gardar