feat(runner/prometheus): PROMETHEUS_AUTH, URL query string, retention fallback - #526
feat(runner/prometheus): PROMETHEUS_AUTH, URL query string, retention fallback#526mayankpande88 wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for several new Prometheus configuration variables, including PROMETHEUS_AUTH for static authorization, PROMETHEUS_URL_QUERY_STRING to append query parameters to Prometheus requests, and PROMETHEUS_RETENTION_TIME as a fallback retention value. It updates the agent runner, Prometheus client, and Grafana proxy to handle these configurations, and extends the retention probe to support VictoriaMetrics. Feedback on the changes points out discrepancies in the documentation and code comments regarding the delimiter for PROMETHEUS_HEADERS (which is parsed as comma-separated, not semicolon-separated), and suggests using net/url instead of manual string manipulation to safely append query parameters.
| |---|---|---| | ||
| | `PROMETHEUS_URL` | recommended | Enables `prometheus_*` actions and `service_map` | | ||
| | `PROMETHEUS_HEADERS` | optional | Comma-separated `Header: value` pairs (e.g. `X-Scope-OrgID: tenant-1`); use for static basic/bearer auth | | ||
| | `PROMETHEUS_HEADERS` | optional | Semicolon-separated `Header: value` pairs (e.g. `X-Scope-OrgID: tenant-1`); use for static basic/bearer auth | |
There was a problem hiding this comment.
The documentation states that PROMETHEUS_HEADERS is semicolon-separated, but the underlying parser config.ParseHeaders in runner/pkg/config/config.go splits headers by commas (,):
for _, part := range strings.Split(s, ",")If users configure this variable using semicolons, it will fail to parse correctly (e.g., the second header will be treated as part of the first header's value). Please revert this to 'Comma-separated' or update the parser to support semicolons.
|
|
||
| // PrometheusHeaders is the parsed PROMETHEUS_HEADERS env (raw | ||
| // "Header: value, Header: value" string) — applied to every | ||
| // "Header: value; Header: value" string) — applied to every |
There was a problem hiding this comment.
This comment describes PROMETHEUS_HEADERS as semicolon-separated (Header: value; Header: value), but config.ParseHeaders actually parses them using commas (,) as delimiters. Please update the comment to reflect the actual comma-separated format to avoid confusion.
| // "Header: value; Header: value" string) — applied to every | |
| // "Header: value, Header: value" string) — applied to every |
| func appendQueryString(rawurl, extra string) string { | ||
| extra = strings.TrimSpace(extra) | ||
| extra = strings.TrimPrefix(extra, "?") | ||
| if extra == "" { | ||
| return rawurl | ||
| } | ||
| sep := "?" | ||
| if strings.Contains(rawurl, "?") { | ||
| sep = "&" | ||
| } | ||
| return rawurl + sep + extra | ||
| } |
There was a problem hiding this comment.
Using manual string manipulation to append query parameters can be fragile (e.g., if the URL already ends with ? or &, or contains fragments). Since net/url is already imported in this file, we can leverage url.Parse to safely and robustly append the query string.
func appendQueryString(rawurl, extra string) string {
extra = strings.TrimSpace(extra)
extra = strings.TrimPrefix(extra, "?")
if extra == "" {
return rawurl
}
u, err := url.Parse(rawurl)
if err != nil {
sep := "?"
if strings.Contains(rawurl, "?") {
sep = "&"
}
return rawurl + sep + extra
}
if u.RawQuery == "" {
u.RawQuery = extra
} else {
u.RawQuery += "&" + extra
}
return u.String()
}
0134581 to
68a929b
Compare
… fallback Restore three prometheus config knobs the legacy agent honored: - PROMETHEUS_AUTH: a static Authorization header applied to every Prometheus request (direct client + UI proxy path). Overlaid via Set so it takes precedence over any Authorization in PROMETHEUS_HEADERS. - PROMETHEUS_URL_QUERY_STRING: appended to the query string of every Prometheus request (direct client + proxy), preserving existing params. - PROMETHEUS_RETENTION_TIME: fallback retention value reported to the UI when /status/flags is unavailable (VictoriaMetrics vmsingle 404s it); also read VM's -retentionPeriod flag key when present. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2HAXQH1gEtX7h4sUJsi9j (cherry picked from commit b3a55fe)
…cedence promHeadersWithAuth had been inserted between esHTTPClient's doc comment and its declaration, so the comment described the wrong function. Also records that falling back to PROMETHEUS_RETENTION_TIME on a successful flags response that lacks both retention keys is an intentional extension of the legacy behavior, not an accident.
4ee4b88 to
8bc63a9
Compare
|
Rebased onto Two fixes while rebasing:
Note this will conflict with #523 on |
|
📦 Image Tags Updated |
1 similar comment
|
📦 Image Tags Updated |
3 participants
Restore three prometheus config knobs the legacy agent honored:
Prometheus request (direct client + UI proxy path). Overlaid via Set
so it takes precedence over any Authorization in PROMETHEUS_HEADERS.
Prometheus request (direct client + proxy), preserving existing params.
when /status/flags is unavailable (VictoriaMetrics vmsingle 404s it);
also read VM's -retentionPeriod flag key when present.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01L2HAXQH1gEtX7h4sUJsi9j