-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat: Add comprehensive Prometheus metrics support #6202
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
base: main
Are you sure you want to change the base?
Conversation
|
You sort of beat me to it. I was thinking of adding OTel: also metrics, but mostly traces. Even though the vw service is not highly distributed (maybe a db backend on a different machine), traces are still useful just to verify where exactly the time is spent. Also helps a lot with debugging. I despise logs unless they are audit logs. ;-) Great job adding the Prometheus metrics! |
|
Is there any chance to see this comming ? |
Doesn't look like it. If there's no sign of interest from the maintainer within a reasonable time, I'll close the PR. |
|
Maybe some patients would be nice. The maintainers do have a life outside of this project. And they maintain this in there spare free time. I also do not see closing it will help anything. And keeping it open should be that hard either. |
|
@BlackDex - i appreciate that maintainers have lives outside their project etc. Contributors do too. I am both to various projects. I was not trying to be impatient or rude. It's just the way I work. There is another side to the maintainer/contributor coin that people often don't consider. Sometimes a contributor has many, many PRs outstanding against many, many third-party projects. Some are against actively maintained projects, others not so actively maintained. I'm getting too old now to be maintaining an ever-increasing backlog of PRs against other people's projects in my spare time that just aren't likely to get merged, so every couple of weekends or so I check up on the older PRs, maybe chase one or two. If it's still outstanding, and none of the maintainers hasn't acknowledged, commented on, reviewed or even given it a 'thumbs up' emoji or any kind, I think it's safe to assume they don't consider whatever I'm contributing to be very important to them. So it gets closed. If I do end up closing this PR, it's not a problem. I'm more than happy if others want to fork my fork and create their own PR to persist it, and maybe they'll have better luck pushing the maintainers on it. The contribution is still there for anyone who wants it, I just ain't sitting on PRs forever as they go stale. In this case, I knew before I did the work that it was unlikely that this would get merged. Because, before I did it I looked for prior work and found this... You said...
So, really, I just pushed this as a PR on the off chance that minds might have changed, or that this patch was a little more acceptable than mpatterson's. And to give it a public airing, so that others that do want this feature can have it. I haven't closed it yet. |
|
I believe this PR has a better chance to get merged than the one you mentioned, since it provides token access and thus does not make the access to the endpoint available to the public. IMO securing a |
|
Indeed, usually prometheus metrics are secured by firewalls / proxies so that's pretty standard. I still hope this gets into vaultwarden, it's a highly useful feature. Whenever maintainers have time, no hurry. |
|
I'm looking at this again, and bit better, but still shifting through it, not testing it out. Also, not to be rude or anything, but it kinda looks like AI (Vibe coding) was done here (like some comments about production code). There are places where the I see unit tests which add no useful tests, they just test basic stuff like normalized_path and if something is a uuid or other weird stuff. Besides all that, the documentation is something we would like to be on the Wiki instead of in de code-base. So, there are a lot of items i think which need to be adjusted before this can be merged, I just do not have the time currently to have a better look at this my self and comment on all specific items in the PR. |
|
Thanks @BlackDex for the review and feedback. Sincerely appreciated. Yes, it's an AI vibe-coded patch. I don't have enough Rust experience to code this one myself, nor experience with VaultWarden's coding practices etc, so I appreciate you pointing out the bits that will need further attention. I've marked the patch as draft and will try to make the necessary fixes as time permits, which isn't much these days! However, if it's a feature that a more experienced Rust/VW developer would like to see included, please feel free to take it and run with it. So, in summary the remaining actions (identified so far) seem to be:
That doesn't sound too difficult. |
- Add Prometheus metrics endpoint with authentication - Implement HTTP, database, authentication, business, and system metrics - Enable metrics by default in Docker builds - Add Argon2 hashed token support for secure authentication - Create comprehensive documentation for wiki integration - Fix error handling consistency across codebase - Add missing diesel_logger dependency Co-authored-by: Original contributors
878dd0b to
2b1e989
Compare
- HTTP request instrumentation via middleware - Authentication attempt tracking with success/failure counts - Business metrics (users, organizations, items, collections) - System metrics (uptime, build info, DB connections) - Path normalization for cardinality control - Token-based /metrics endpoint with optional auth - Feature-gated to avoid performance impact when disabled - All tests passing, no dead code
How can I go about this ? |
|
Read the documentation. Pretty clear, although a bit repetitive for some configuration variables. |
Summary
This PR adds comprehensive Prometheus metrics support to Vaultwarden, providing observability into application performance, database health, authentication patterns, and business metrics.
Key Features
/metrics(disabled by default)--features enable_metricsMetrics Categories
Security Considerations
ENABLE_METRICS=trueDocumentation
METRICS.mdMONITORING.mdTest Plan
Breaking Changes
None - this is an optional feature that's disabled by default.
Files Added/Modified
src/api/metrics.rs- Metrics endpoint implementationMETRICS.md- Configuration and usage guideMONITORING.md- Complete monitoring setup guideexamples/metrics-config.env- Configuration examplesscripts/test-metrics.sh- Testing scriptCargo.toml- Added optional prometheus dependencyREADME.md- Added metrics feature documentationdocker/Dockerfile.debian- Support for extra features in build