-
Notifications
You must be signed in to change notification settings - Fork 3.9k
[telemetry] support prometheus sink #18336
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
Conversation
c408abc to
d795e3c
Compare
82accfa to
df61586
Compare
df61586 to
a8c9dc0
Compare
| ) | ||
| } | ||
| }, | ||
| SinkAuthType::None => victoria_metrics::AuthToken::Bearer("".to_string()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metrics clients send empty Bearer auth when auth_type is none
Medium Severity
When auth_type: none is configured, metrics clients (VictoriaMetricsClient, PrometheusRemoteWriteClient) still send an Authorization: Bearer header with an empty token. This happens because SinkAuthType::None maps to AuthToken::Bearer("") instead of truly disabling authentication. The Loki client correctly handles this with a LokiAuth::None variant that skips adding any auth header, but the metrics AuthToken enum lacks a None variant. This inconsistency could cause issues with strict servers that reject malformed auth headers.
Additional Locations (2)
crates/aptos-telemetry-service/src/clients/prometheus_remote_write.rs
Outdated
Show resolved
Hide resolved
a8c9dc0 to
e032be9
Compare
JoshLind
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unblocking!
(Big boss @ibalajiarun can give the final stamp 😄 Most of this is dark magic to me.)
| serde = { workspace = true } | ||
| serde_json = { workspace = true } | ||
| serde_yaml = { workspace = true } | ||
| snap = "1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't want to use the workspace version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only used in this crate actually, so doesn't make sense to include it in the workspace
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|

Description
Support prometheus and prometheus compatible metrics sinks via remote write. Previously only VictoriaMetrics prometheus import endpoint was supported
How Has This Been Tested?
Key Areas to Review
New prometheus remote write sink
Type of Change
Which Components or Systems Does This Change Impact?
Checklist
Note
Introduces a Prometheus Remote Write metrics sink and refactors ingestion to support multiple backends with flexible auth.
prometheus_remote_writeclient (protobuf + snappy) and unifiedMetricsIngestClient; refactors VictoriaMetrics clientbackend_type(victoria_metrics|prometheus_remote_write / humio|loki),auth_type(bearer|basic|none),keys_env_var/basic_auth_env_var, and support bothmetrics_sinkandmetrics_sinks/api/v1/healthendpoint; update tests to use new clients; minor renames/struct movesprostandsnapWritten by Cursor Bugbot for commit a8c9dc0. This will update automatically on new commits. Configure here.