Add Prometheus metrics for auth validation - #184
Conversation
- New module aws_auth_validate_metrics implementing prometheus_collector
behaviour (pull-based, matching rabbitmq_shovel_prometheus idiom)
- Uses OTP counters with write_concurrency + persistent_term for
lock-free atomic increments on the request hot path
- Metrics: requests_total (counter, by method+result),
duration_milliseconds (histogram, by method),
capacity_exhausted_total (counter, by method),
semaphore_in_use (gauge), semaphore_capacity (gauge)
- Wired observe/3 into audit/5 in aws_auth_validate_mgmt so audit
and metrics stay in lockstep at the single choke point
- Added usage/0 to aws_auth_validate_semaphore for gauge reads at
scrape time
- Registration gated on auth_validation_enabled in aws_sup; no metric
state when feature is off
- Common Test suite covering counter increments, histogram buckets,
capacity_exhausted, semaphore gauges, disabled-state no-op, and
crash safety
- Documented in AUTH_VALIDATION.md Metrics section
sdewhitt
added
C-enhancement
- aws_auth_validate_metrics uses prometheus_registry and the
prometheus_collector behaviour, but prometheus was not in DEPS; it
resolved only because another plugin in the build tree pulls it in.
Add it to DEPS so it is a declared runtime dependency, matching
rabbitmq_prometheus.
- prometheus is a declared dependency, so a registration error is a real
fault; drop the try/catch that swallowed it silently.
38e133b to
c25c10e
Compare
sdewhitt
added
P-low
lukebakken
left a comment
lukebakken
left a comment
There was a problem hiding this comment.
Thanks for adding a pull-based Prometheus collector for the auth-validation endpoint. The slot layout and the no-op-when-unregistered guard are clean. A few issues need addressing before merge, most severity in the duration histogram and scrape robustness. Details inline; findings 1-3 are the blockers, 4 is a real metric-quality bug, 5-6 are lower priority.
Verified locally at deps/aws on feature/auth-validation-metrics (HEAD c25c10e), including that prometheus_model_helpers:histogram_buckets/1 maps buckets 1:1 with no auto-appended +Inf.
- Depend on rabbitmq_prometheus (which transitively pulls in prometheus) rather than the bare prometheus library, matching how rabbitmq-stream-s3 does it -- guarantees the /api/metrics scrape endpoint is actually served - Update the aws_sup comment that referenced the old dependency
…nd scrape Metric correctness (Group A): - A1: Add overflow bucket (position 11) for durations > 10000ms and emit le="+Inf" as the final cumulative bucket (Prometheus requirement); NUM_BUCKETS=11, SLOTS_PER_HIST=13 - A2: Track unrecognized method paths under a synthetic method="unknown" label in requests_total (fixed cardinality, no raw string leak); NUM_METHODS=5 - A4: Gate duration recording by category -- only categories that represent actual backend/outbound work (success, auth_failed, connection_failed, tls_failed, authz_unverified, token_expired, token_invalid) contribute to the histogram; pre-connection rejects (input_invalid, body_too_large, config_conflict, capacity_exhausted, method_disabled, unknown_method, internal_error, query_invalid) are excluded. query_invalid is excluded because query parsing happens inside parse_input (step 1 of validate/1), before any eldap:open. Scrape robustness (Group B): - B3: Add catch-all clause in try_semaphore_usage/0 so an arbitrary exit degrades to unavailable rather than crashing the whole /api/metrics scrape - B5: Make register/0 get-or-create -- only allocate counters when absent; a second call (e.g. supervisor restart) preserves existing values Cleanup (Group C): - C6: Replace find_bucket_pos coupling in build_cumulative_buckets/2 with direct positional enumeration via lists:zip; layout is expressed once in the macros and cannot drift Tests: 13 CT cases covering all new behaviour (inf_bucket_exists, duration_gating_excludes_fast_rejects, unknown_method_counted, semaphore_exit_catch_all, register_preserves_counters, plus existing).
lukebakken
left a comment
lukebakken
left a comment
There was a problem hiding this comment.
All six findings from the earlier review are addressed in 097938e, each with test coverage, and the prometheus dependency now goes through rabbitmq_prometheus per the Makefile thread. Approving.
- +Inf/overflow bucket added (position 11); build_cumulative_buckets/2 emits {infinity, sample_count}, which renders as le="+Inf"
- unknown methods counted under a fixed method="unknown" label (no raw-string cardinality risk)
- try_semaphore_usage/0 has a : catch-all so an arbitrary semaphore exit no longer fails the whole scrape
- duration histogram gated by is_timed_category/1 so pre-connection rejects no longer skew latency
- register/0 is get-or-create, preserving counts across re-registration
- build_cumulative_buckets/2 enumerates slots directly instead of round-tripping through find_bucket_pos/1
Verified locally at deps/aws on feature/auth-validation-metrics (HEAD 097938e): compile, dialyzer (after a PLT rebuild, since the dependency set changed), eunit (183), and ct-aws_auth_validate_metrics (13 Ok / 0 failed) all pass.
|
Actually I was curious if we could have rabbitmq-stream-s3 depend on We don't call anything within the rabbitmq_prometheus plugin directly. IIRC the collector is registered during boot - something scans for modules which declare the prometheus_collector behaviour. |
|
Another example: cloudamqp/rabbitmq-delayed-message-exchange#17 |
|
Ah OK. Well, I don't think it's too much of a hassle / hardship since |
3 participants
Adds Prometheus metrics for the auth-validation endpoint so operators can observe call volume, outcome mix, latency, and capacity pressure.
Metrics are emitted at the single point every request passes through (the audit log call site), dimensioned only by method and the fixed result-category set. No source IP, user, target, or secret material is ever used as a label.
Emitted via the
prometheus_collectorbehaviour (the same pull-based pattern other RabbitMQ plugins use): the hot path is a lock-free counter update, and formatting happens only on scrape...._requests_total(counter),..._duration_ms(histogram),..._capacity_exhausted_total(counter)Adds new dependency,
prometheus, to DEPS. However, it is already listed as a dependency in rabbitmq_prometheus, which already exists within rabbitmq-server.Tests
CT suite covering counter increments per outcome, duration recording, capacity signalling, gauge behaviour, disabled-state, and crash-safety. All green.