Open
Conversation
… push lifecycle, security, file upload, and user authentication metrics
|
Hello @cgrard, thank you for submitting a PR! We will respond as soon as possible. |
Owner
|
Hi @cgrard - this is a great idea - thanks for creating! I'm familiar with Prometheus but haven't used it in a few years. I'll take a closer look very soon. |
The subject_name method was incorrectly placed in the private section after adding Prometheus tracking callbacks, causing errors when the method was called from view templates (app/views/audit_logs/_log_creation.html.erb). Moved subject_name back to the public section to restore proper access from views while keeping Prometheus tracking methods private. Fixes 16 failing tests across OwnerAndAdminViewTest, QrAuditTest, AuditLogTest, and PasswordAuditTest.
Author
|
Hence the importance of unit tests ! It turns out I actually broke something by modifying the scope without realizing it, but it has now been fixed and all tests pass successfully on a |
Owner
|
Hi @cgrard - just a heads up that I haven't forgotten about this PR. I want to push to merge this sometime soon. Thanks for the patience! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
First I'd like to thank you for this amazing app, it is really very useful and very well done.
I've been using PasswordPusher for quite a while (everyone loves it) and it is currently deployed in a Kubernetes environnement where I felt the need to improve monitoring and observability lately, so I wondered if there was some kind of
/metricsexposed somehow and found out that there isn't. I haven't worked with Ruby in years, and it has never been my primary language, so I apologize in advance if the code is not of the highest quality, I did my best.I will probably also provide a Grafana Dashboard at some point if anyone is interested as it is a pretty popular observability ecosystem.
TL;DR This PR adds comprehensive Prometheus metrics export functionality to Password Pusher, enabling monitoring, observability, and security analytics.
Features implemented:
prometheus_exportergemArchitecture:
Related Issue
Neither related issue nor implementation request
Type of Change
Checklist
rake test)Additional Notes
New files:
config/prometheus_server.rb- Prometheus exporter server with custom collectorsconfig/initializers/prometheus.rb- Client configuration and instrumentationconfig/initializers/devise_prometheus.rb- Devise authentication event trackingapp/models/concerns/prometheus_metrics.rb- Reusable metrics tracking concerntest/prometheus_test.rb- TestsPROMETHEUS.md- Setup and deployment documentationMETRICS.md- Complete metrics reference (600+ lines with queries, dashboards, alerts)Not sure about the .md documentation, I think that both files should be converted into a specific chapter in the documentation and be removed from the repo, don't they?
Modified files:
Gemfile- Addedprometheus_exportergemProcfile/Procfile.dev- Added prometheus processapp/models/push.rb- Added metrics tracking callbacksapp/models/audit_log.rb- Added view and security metrics trackingapp/models/user.rb- Added authentication metrics tracking**Testing:** Tests have not been written for this PR as: - Prometheus metrics are disabled in the test environment (`Rails.env.test?`) - The feature is non-invasive and fails gracefully if the exporter is unavailable - Comprehensive manual testing has been performed in development environment - Metrics tracking uses simple ActiveRecord callbacks without complex logicTests could be added in a follow-up PR if desired, potentially mocking the
PrometheusExporter::Clientto verify correct metric tracking.What do you think?