fix(plugin-prometheus): Use appendPath to build URI in Prometheus connector #26733
Conversation
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
In case of a prometheus server behind proxy or a prometheus compatible API (e.g. VictoriaMetrics), prometheus API URI should be appended. Similar to what has been done in: trinodb/trino@55c008b
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideSwitches Prometheus connector HTTP URI construction to use HttpUriBuilder.appendPath so that API paths are appended to the configured base URI instead of replacing existing paths, improving compatibility with proxied/Prometheus-compatible endpoints. Sequence diagram for Prometheus query URI construction with appendPathsequenceDiagram
participant PrometheusSplitManager
participant HttpUriBuilder
participant URIBuilder
participant HttpClient
PrometheusSplitManager->>HttpUriBuilder: uriBuilderFrom(baseURI)
HttpUriBuilder-->>PrometheusSplitManager: URIBuilder
PrometheusSplitManager->>URIBuilder: appendPath(api_v1_query)
PrometheusSplitManager->>URIBuilder: addParameter(query, metricName_with_range)
PrometheusSplitManager->>URIBuilder: addParameter(time, time)
PrometheusSplitManager->>URIBuilder: build()
URIBuilder-->>PrometheusSplitManager: queryURI
PrometheusSplitManager->>HttpClient: execute(queryURI)
HttpClient-->>PrometheusSplitManager: Prometheus_response
Flow diagram for Prometheus base URI composition with appendPathgraph TD
A["Configured_prometheus_base_URI
(e.g. http://ip:port/select/1:1/prometheus)"] --> B["getPrometheusMetricsURI
PrometheusClient"]
B --> C["HttpUriBuilder.uriBuilderFrom(prometheusUri)"]
C --> D["appendPath(METRICS_ENDPOINT)
(e.g. /api/v1/label/__name__/values)"]
D --> E["build()
metrics_URI
(e.g. http://ip:port/select/1:1/prometheus/api/v1/label/__name__/values)"]
A --> F["buildQuery
PrometheusSplitManager"]
F --> G["HttpUriBuilder.uriBuilderFrom(baseURI)"]
G --> H["appendPath(api/v1/query)"]
H --> I["addParameter(query, metric_with_range)"]
I --> J["addParameter(time, time)"]
J --> K["build()
query_URI
(e.g. http://ip:port/select/1:1/prometheus/api/v1/query)"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The
buildQuerymethod no longer constructs URIs in a way that can throwURISyntaxException, so you can drop thethrows URISyntaxExceptionfrom its signature and simplify any call sites that were forced to handle it. - When switching from
replacePathtoappendPath, double-check thatappendPath("api/v1/query")correctly handles cases where the base URI path already ends with or without a trailing slash, to avoid unintended//or missing separators in the final path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `buildQuery` method no longer constructs URIs in a way that can throw `URISyntaxException`, so you can drop the `throws URISyntaxException` from its signature and simplify any call sites that were forced to handle it.
- When switching from `replacePath` to `appendPath`, double-check that `appendPath("api/v1/query")` correctly handles cases where the base URI path already ends with or without a trailing slash, to avoid unintended `//` or missing separators in the final path.
## Individual Comments
### Comment 1
<location> `presto-prometheus/src/main/java/com/facebook/presto/plugin/prometheus/PrometheusClient.java:96` </location>
<code_context>
private static URI getPrometheusMetricsURI(URI prometheusUri)
{
- try {
- // endpoint to retrieve metric names from Prometheus
- return new URI(prometheusUri.getScheme(), prometheusUri.getAuthority(), prometheusUri.getPath() + METRICS_ENDPOINT, null, null);
- }
- catch (URISyntaxException e) {
- throw new RuntimeException(e);
- }
+ // endpoint to retrieve metric names from Prometheus
+ return HttpUriBuilder.uriBuilderFrom(prometheusUri).appendPath(METRICS_ENDPOINT).build();
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Preserving query/fragment from the base URI may change the effective metrics endpoint.
The old code built a URI from only scheme, authority, and `path + METRICS_ENDPOINT`, dropping any query/fragment from `prometheusUri`. The new use of `uriBuilderFrom(prometheusUri)` keeps query/fragment/user-info and then appends the metrics path, so any configured query params or fragment will now be sent to the metrics endpoint. If the intent is a clean metrics URL, consider clearing query/fragment when building it (e.g., via `replacePath` plus explicitly clearing query).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| throw new RuntimeException(e); | ||
| } | ||
| // endpoint to retrieve metric names from Prometheus | ||
| return HttpUriBuilder.uriBuilderFrom(prometheusUri).appendPath(METRICS_ENDPOINT).build(); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Preserving query/fragment from the base URI may change the effective metrics endpoint.
The old code built a URI from only scheme, authority, and path + METRICS_ENDPOINT, dropping any query/fragment from prometheusUri. The new use of uriBuilderFrom(prometheusUri) keeps query/fragment/user-info and then appends the metrics path, so any configured query params or fragment will now be sent to the metrics endpoint. If the intent is a clean metrics URL, consider clearing query/fragment when building it (e.g., via replacePath plus explicitly clearing query).
Contributor
|
Please update the Release Notes section of the Description so that this PR can pass the failing test. https://github.com/prestodb/presto/actions/runs/19885919869/job/56993039935?pr=26733 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
In case of a prometheus server behind proxy or a prometheus compatible API (e.g. VictoriaMetrics), prometheus API URI should be appended. Similar to what has been done in: trinodb/trino@55c008b
Motivation and Context
With replacePath, query will return 404 on VictoriaMetrics (we are using its prometheus API and configured as: http://ip:port/select/1:1/prometheus) as current code base will build the URI as http://ip:port/api/v1/query
Impact
Test Plan
Release Notes