Skip to content

Comments

Introduce controleval_metrics.py tool to generate metrics in Prometheus format#11040

Merged
mildas merged 13 commits intoComplianceAsCode:masterfrom
marcusburghardt:controleval_prometheus
Sep 7, 2023
Merged

Introduce controleval_metrics.py tool to generate metrics in Prometheus format#11040
mildas merged 13 commits intoComplianceAsCode:masterfrom
marcusburghardt:controleval_prometheus

Conversation

@marcusburghardt
Copy link
Member

Description:

In the previous state of controleval.py we can easily collect stats about control files.
This is pretty useful to check the current state.
However, it would be also very useful to have metrics showing the history of policies so we can easily see how it is progressing, maintained, etc. To achieve this, the information should be periodically collected and ideally stored in a time series database.

Prometheus is a pretty popular and straightforward tool to collect metrics in a time series database which can be easily integrated with Grafana to show nice dashboards, like it is done in the CommunityMon project.

This PR extends the controleval.py tool to use the already collected metrics and export them in the Prometheus format.

Rationale:

  • Better resources to monitor control files requirements
  • Enable easy integration of control files metrics in CommunityMon project or any other instance of Prometheus

Review Hints:

Please, check the commits and their respective commit messages as they are ordered.
To see how the metrics are exported, here is an example for policies used by rhel9 and rhel8:

$ utils/controleval.py prometheus -p rhel9 rhel8 -f /tmp/policies_metrics
$ cat /tmp/policies_metrics

@marcusburghardt marcusburghardt added this to the 0.1.70 milestone Aug 30, 2023
@marcusburghardt marcusburghardt added enhancement General enhancements to the project. Infrastructure Our content build system New Feature Issues or pull requests related to new Features. and removed Infrastructure Our content build system labels Aug 30, 2023
@github-actions
Copy link

Start a new ephemeral environment with changes proposed in this pull request:

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@Mab879 Mab879 self-assigned this Aug 30, 2023
Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I do have a couple comments.

@Mab879
Copy link
Member

Mab879 commented Aug 31, 2023

The CI failures look legit. Seems that we are not using requirements.txt in some of the tests.

Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script looks good, but the CI failures need attention.

@marcusburghardt
Copy link
Member Author

/packit build

Copy link
Contributor

@mildas mildas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As prometheus_client is not a project requirement and we don't want it to be, the problematic ctests should be run only when the prometheus_client is installed on the system.

In the TF tests, it makes sense to install the dependency and test it. However, for customers and contributors who don't want it, in the current state it will cause failing ctests and they will be forced to install the dependency on their system just to make it pass.

@mildas
Copy link
Contributor

mildas commented Sep 5, 2023

/packit retest-failed

@marcusburghardt
Copy link
Member Author

marcusburghardt commented Sep 5, 2023

Now the only PR blocking this new feature is #11048

Discussing with @mildas , we concluded it would be better to import prometheus-client conditionally only when the prometheus subcmd is called. However, it would make the code less readable. So, my plan is move the prometheus related functions to a new file in utils folder.

Update: It is done by the last two commits.

Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script looks good.

Please include a conditional ctest that will run the Prometheus format if prometheus_client is installed.

@marcusburghardt
Copy link
Member Author

The script looks good.

Please include a conditional ctest that will run the Prometheus format if prometheus_client is installed.

@Mab879 I wasn't planning to include tests for this script for now. Once this PR is merged I will propose a new PR to integrate it with GitHub Pages and then implement all necessary tests at once. If you have objections I can include the test in this PR tomorrow.

@Mab879
Copy link
Member

Mab879 commented Sep 6, 2023

The script looks good.
Please include a conditional ctest that will run the Prometheus format if prometheus_client is installed.

@Mab879 I wasn't planning to include tests for this script for now. Once this PR is merged I will propose a new PR to integrate it with GitHub Pages and then implement all necessary tests at once. If you have objections I can include the test in this PR tomorrow.

That will work for me.

@mildas Is everything okay on this PR?

Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Sep 7, 2023
Moved and split code in specific functions to make them more usable in
other functions or subcommands.
This commit creates the subcmd, its options and a very basic function
which will evolve in the next commits.
The goals is to collect prometheus metrics from all control files used
by the informed products. Therefore, the function iterates with the list
of products, locate all profiles files and search for controls used in
profile files. If a control is found, a set is incremented to further
usage. Once all relevant control files are identified, stats are
collected from each specific level in a policy. This commit cares of
collectiong the necessary information. Next commits will implement the
Prometheus format.
Once the data necessary for the metrics is collected, a Prometheus
registr is created and populated with metrics for all policies used by
informed products. Finally the registry is exported in a txt file in
Prometheus format in order to be easily consumed by Prometheus jobs.
Shorter lines with better readability in functions.
The initial behaviour was expecting to save the metrics in a file by
using the write_to_textfile function from prometheus_client. There was
no specific function to redirect the output to stdout. However, it was
found another funcion which returns the metrics encoded in utf-8. This
function was used to make the stdout as default.
This script depends on controleval.py but is intended to generate
metrics to be consumed by other tools. In this initial version, it is
generating metrics for Prometheus but other tools can be included in the
future, if necessary. These functions were initially included in
controleval.py but the drawnback was that prometheus_client module was
required by the script even when calling the stats sub-command and
making the module conditional would impact the readbility.
These functions were moved to controleval_metrics.py.
If the prometheus-client module is present, the controleval_metrics.py
will be tested.
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Used by openshift-ci bot. label Sep 7, 2023
@marcusburghardt
Copy link
Member Author

@mildas in this meantime while in review I managed to include a conditional test (98ad1a0) for prometheus_metrics.py.
After that, I needed to rebase in order to solve a conflict in CMakeLists.txt

@marcusburghardt marcusburghardt changed the title Extend controleval.py tool to generate metrics in Prometheus format Introduce controleval_metrics.py tool to generate metrics in Prometheus format Sep 7, 2023
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 98ad1a0 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 53.8% (0.0% change).

View more on Code Climate.

Copy link
Contributor

@mildas mildas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked the code and performed testing, LGTM

@mildas mildas merged commit c382b19 into ComplianceAsCode:master Sep 7, 2023
@marcusburghardt marcusburghardt deleted the controleval_prometheus branch September 7, 2023 14:48
@Mab879 Mab879 added the Infrastructure Our content build system label Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement General enhancements to the project. Infrastructure Our content build system New Feature Issues or pull requests related to new Features.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants