Skip to content
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

RT-258 Add Prometheus Metrics exports #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

joao-vieira-leao-reef
Copy link

Intent:
This is part of RT-258. The other part will soon be another pull request to the Django Template.

Summary of changes:

We're exporting metrics to a customizable (through env vars) directory in the Prometheus .prom format. We have a default metric that states the last scan and push that occurred. Apart from it, there is already groundwork for making new metrics on the fly should we need to.

We're exporting metrics to a customizable (through env vars) directory in the Prometheus .prom format.
We have a default metric that states the last scan and push that occurred. Apart from it, there is already groundwork for making new metrics on the fly should we need to.
Copy link

@emnoor-reef emnoor-reef left a comment

Choose a reason for hiding this comment

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

The solution looks good. But IMO it is unnecessarily abstracted. A simple function that takes a path and a value (timestamp) and writes to the prom file would have been much better.

Unless there are other metrics planned to be exported, I don't see the value of abstraction from the get go.

src/vulnrelay/metrics/exporter.py Outdated Show resolved Hide resolved
src/vulnrelay/metrics/exporter.py Outdated Show resolved Hide resolved
- removed metric class, as it was holding a single value
- switched to pathlib in exporter check_dir
- removed redundant file deletion call with shutil
@joao-vieira-leao-reef
Copy link
Author

The solution looks good. But IMO it is unnecessarily abstracted. A simple function that takes a path and a value (timestamp) and writes to the prom file would have been much better.

Unless there are other metrics planned to be exported, I don't see the value of abstraction from the get go.

I agree in retrospect. I still kept some abstractions though, I believe this can benefit from it as more metrics are added should the demand arise. Still, I deleted the Metric class, since it was essentially holding a single value haha.

src/vulnrelay/metrics/exporter.py Show resolved Hide resolved
src/vulnrelay/metrics/exporter.py Outdated Show resolved Hide resolved
src/vulnrelay/metrics/exporter.py Outdated Show resolved Hide resolved
src/vulnrelay/services.py Show resolved Hide resolved
src/vulnrelay/metrics/__init__.py Outdated Show resolved Hide resolved
- removed redudant dict argument to metric exporter
- switched to pathlib on more instances
- added metric exporter test
@joao-vieira-leao-reef
Copy link
Author

made the requested changes (or I hope)

also pushed from wrong account sorry

from vulnrelay.metrics.metric import MetricNames


def test_export_metrics():
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't test much without actually verifying the output file contents

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants