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

feat: prometheus monitoring #10

Merged
merged 6 commits into from
Aug 2, 2024
Merged

Conversation

manu0466
Copy link

Description

This PR allows to expose a /metrics endpoint that can be used to monitor a parser trough prometheus.

The monitoring can be enabled trough the config file by adding a new monitoring section like this:

monitoring:
  enabled: true
  port: 2112

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Re-reviewed Files changed in the Github PR explorer.

@manu0466 manu0466 self-assigned this Jul 31, 2024
@RiccardoM
Copy link
Collaborator

Can you please fix the linting errors?

cmd/start/cmd.go Outdated Show resolved Hide resolved
@manu0466 manu0466 requested a review from RiccardoM August 2, 2024 08:42
@@ -35,5 +37,6 @@ func NewContext(
Database: db,
Modules: modules,
Logger: logger,
Prometheus: prometheus.NewServer(config.Monitoring.Port),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't this case an error if config.Monitoring is null (i.e. it was not sent inside the config? I think we should just return a nil Prometheus instead. We could just pass config.Monitoring to the NewServer method and return a nil instance if the monitoring is either disabled or the config is not set at all. Then, inside the main we just call .Start with a nil check and if the server is nil then we do nothing. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I was forcing the definition of the monitoring section inside the config. We can make it optional, as you suggest, to maintain backward compatibility.

@manu0466 manu0466 requested a review from RiccardoM August 2, 2024 14:36
@RiccardoM RiccardoM merged commit d10e918 into main Aug 2, 2024
3 checks passed
@RiccardoM RiccardoM deleted the manuel/prometheus-monitoring branch August 2, 2024 15:59
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.

2 participants