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

Add Prometheus metrics for DarkBlue Service #48

Merged
merged 29 commits into from
May 17, 2024

Conversation

jayamala17
Copy link
Collaborator

@jayamala17 jayamala17 commented May 1, 2024

  • Added Prometheus pushgateway used for monitoring batch or cron jobs.
  • Through the gateway url endpoint the metrics are pushed to prometheus
  • Added metrics for job duration, last job run. and metrics regards to success and failure count on each job run.

@ssciolla
Copy link
Collaborator

ssciolla commented May 9, 2024

Hi @jayamala17, could you resolve the conflicts, improve the title, and add a short description?

@ssciolla ssciolla added the enhancement New feature or request label May 9, 2024
@jayamala17 jayamala17 changed the title Added loglevel method Add Prometheus metrics for DarkBlue Service May 9, 2024
Copy link
Collaborator

@ssciolla ssciolla left a comment

Choose a reason for hiding this comment

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

Releasing some comments so you can see what I've thought about so far. I'm going to think a bit more about the tests (the stubbing and mocking), but probably tomorrow. I will also give it a whirl then.

Gemfile Outdated Show resolved Hide resolved
lib/jobs.rb Outdated Show resolved Hide resolved
lib/jobs.rb Outdated Show resolved Hide resolved
lib/jobs.rb Outdated Show resolved Hide resolved
lib/jobs.rb Outdated Show resolved Hide resolved
lib/jobs.rb Outdated Show resolved Hide resolved
lib/jobs.rb Outdated Show resolved Hide resolved
run_dark_blue.rb Outdated Show resolved Hide resolved
test/test_jobs.rb Outdated Show resolved Hide resolved
test/test_jobs.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@ssciolla ssciolla left a comment

Choose a reason for hiding this comment

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

Releasing some comments. Still looking at this and I will work on that separate branch with my config suggestions.

Gemfile Outdated Show resolved Hide resolved
example.env Outdated Show resolved Hide resolved
lib/config.rb Outdated Show resolved Hide resolved
test/test_jobs.rb Outdated Show resolved Hide resolved
lib/jobs.rb Outdated Show resolved Hide resolved
lib/jobs.rb Outdated Show resolved Hide resolved
lib/jobs.rb Outdated Show resolved Hide resolved
lib/status_event_repository.rb Outdated Show resolved Hide resolved
lib/jobs.rb Outdated Show resolved Hide resolved
test/test_jobs.rb Outdated Show resolved Hide resolved
lib/config.rb Show resolved Hide resolved
lib/metrics_collector.rb Outdated Show resolved Hide resolved
lib/metrics_collector.rb Outdated Show resolved Hide resolved
lib/metrics_collector.rb Outdated Show resolved Hide resolved
run_dark_blue.rb Outdated Show resolved Hide resolved
test/test_metrics_collector.rb Outdated Show resolved Hide resolved
test/test_metrics_collector.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@ssciolla ssciolla left a comment

Choose a reason for hiding this comment

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

Sorry, more comments, we're getting closer.

lib/status_event_repository.rb Outdated Show resolved Hide resolved
lib/status_event_repository.rb Outdated Show resolved Hide resolved
lib/metrics_collector.rb Outdated Show resolved Hide resolved
lib/metrics_collector.rb Outdated Show resolved Hide resolved
lib/metrics_collector.rb Outdated Show resolved Hide resolved
test/test_metrics_collector.rb Outdated Show resolved Hide resolved
test/test_metrics_collector.rb Outdated Show resolved Hide resolved
test/test_metrics_collector.rb Outdated Show resolved Hide resolved
test/test_status_event_repository.rb Outdated Show resolved Hide resolved
test/test_status_event_repository.rb Outdated Show resolved Hide resolved
example.env Outdated Show resolved Hide resolved
@ssciolla
Copy link
Collaborator

I got this working on my local machine, looks cool! 🙂

@ssciolla
Copy link
Collaborator

@jayamala17, FYI, I started experimenting with the Mock stuff, found a couple other things worth considering. I'm going to open a PR with the changes so it's easier to talk about. We can discuss tomorrow.

lib/metrics.rb Outdated Show resolved Hide resolved
Copy link
Collaborator

@ssciolla ssciolla left a comment

Choose a reason for hiding this comment

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

Looks good! A couple more ideas to consider, but you can feel free to merge when you would like.

lib/status_event_repository.rb Show resolved Hide resolved
@@ -35,7 +36,7 @@ def initialize(config)
type: config.aptrust.remote.type,
settings: config.aptrust.remote.settings
),
status_event_repo: StatusEventRepository::StatusEventRepositoryFactory.for(use_db: DB),
status_event_repo: S.status_event_repo,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not do this now, but I'd like to get to a place where we only use S in the top-level scope, meaning we'd have to add a new constructor parameter.

test/test_metrics.rb Show resolved Hide resolved
test/test_metrics.rb Outdated Show resolved Hide resolved
@jayamala17 jayamala17 merged commit b4ca813 into main May 17, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants