prometheus: add support for external Prometheus URLs#494
prometheus: add support for external Prometheus URLs#494Tusharjamdade wants to merge 7 commits intoheadlamp-k8s:mainfrom
Conversation
d46e242 to
816ce8a
Compare
|
Hi @illume, could you please review this when you have a moment? |
There was a problem hiding this comment.
Pull request overview
This PR extends the Prometheus plugin so clusters can use either in-cluster Kubernetes services or external Prometheus endpoints configured via full HTTP/HTTPS URLs, while updating the settings UI and request logic accordingly.
Changes:
- Update
getPrometheusPrefixto resolve either a full HTTP/HTTPS Prometheus URL or a Kubernetesnamespace/service:portaddress into the prefix used for querying. - Enhance
fetchMetricsto detect when the prefix is an external URL and route requests directly viafetchinstead of the Kubernetes API proxy, including integration with the optionalsubPath. - Broaden settings validation and helper text to accept and guide users toward both
namespace/service:portand HTTP/HTTPS URL formats for the Prometheus address.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
prometheus/src/util.ts |
Adjusts cluster configuration helpers so Prometheus prefixes can be resolved from either an HTTP/HTTPS URL or a Kubernetes service-style address; also still provides interval, resolution, and subpath helpers used by charts. |
prometheus/src/request.tsx |
Adds URL detection and extends fetchMetrics to support direct calls to external Prometheus URLs while preserving the existing in-cluster Kubernetes proxy behavior. |
prometheus/src/components/Settings/Settings.tsx |
Expands settings validation and UI copy to accept both Kubernetes service addresses and full HTTP/HTTPS URLs for configuring the Prometheus endpoint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (data.query) { | ||
| params.append('query', data.query); | ||
| } | ||
| var url = `/api/v1/namespaces/${data.prefix}/proxy/api/v1/query_range?${params.toString()}`; | ||
| if (data.subPath && data.subPath !== '') { | ||
| if (data.subPath.startsWith('/')) { | ||
| data.subPath = data.subPath.slice(1); | ||
|
|
||
| const isExternal = isHttpUrl(data.prefix); | ||
| var url: string; | ||
|
|
There was a problem hiding this comment.
The implementation of fetchMetrics now supports both in-cluster Kubernetes proxy prefixes and full HTTP/HTTPS URLs (via isHttpUrl), but the JSDoc above still documents data.prefix only as a "namespace prefix". To keep the API contract clear for future callers, consider updating the JSDoc to explicitly describe that prefix can be either a Kubernetes proxy prefix (e.g. namespace/services/service:port) or a full Prometheus base URL, and how that interacts with the optional subPath.
| export async function getPrometheusPrefix(cluster: string): Promise<string | null> { | ||
| // check if cluster has autoDetect enabled | ||
| // if so return the prometheus pod address | ||
| const clusterData = getClusterConfig(cluster); | ||
| if (clusterData?.autoDetect) { | ||
| const prometheusEndpoint = await isPrometheusInstalled(); | ||
| if (prometheusEndpoint.type === KubernetesType.none) { | ||
| return null; | ||
| if (clusterData?.address) { | ||
| const address = clusterData.address.trim().replace(/\/$/, ''); | ||
|
|
||
| if (address.startsWith('http://') || address.startsWith('https://')) { | ||
| return address; | ||
| } | ||
| const prometheusPortStr = prometheusEndpoint.port ? `:${prometheusEndpoint.port}` : ''; | ||
| return `${prometheusEndpoint.namespace}/${prometheusEndpoint.type}/${prometheusEndpoint.name}${prometheusPortStr}`; | ||
| } | ||
|
|
||
| if (clusterData?.address) { | ||
| const [namespace, service] = clusterData?.address.split('/'); | ||
| return `${namespace}/services/${service}`; | ||
| const parts = address.split('/'); | ||
| if (parts.length === 2) { | ||
| const [namespace, service] = parts; | ||
| return `${namespace}/services/${service}`; | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
getPrometheusPrefix now contains additional logic for handling full HTTP/HTTPS URLs and parsing manual Kubernetes service addresses, but there are no unit tests covering this behavior even though util.ts already has tests for other helpers (see util.test.ts). To avoid regressions (especially around URL vs. namespace/service handling and edge cases like trailing slashes or malformed addresses), consider adding focused tests for this function exercising both external-URL and in-cluster service configurations.
There was a problem hiding this comment.
This is a nice to have, but shouldn't block merging IMHO.
There was a problem hiding this comment.
Thanks for this.
Could you please check the open review comments?
Please see the git commit guidelines and look at other git log messages.
@Tusharjamdade It seems there is also a git conflict. Can you please rebase and fix the conflicts?
Signed-off-by: Tusharjamdade <tusharnjamdade@gmail.com>
Signed-off-by: Tusharjamdade <tusharnjamdade@gmail.com>
Signed-off-by: Tusharjamdade <tusharnjamdade@gmail.com>
816ce8a to
a381dbf
Compare
Signed-off-by: Tusharjamdade <tusharnjamdade@gmail.com>
ab1df7f to
eda577e
Compare
Signed-off-by: Tusharjamdade <tusharnjamdade@gmail.com>
Signed-off-by: Tusharjamdade <tusharnjamdade@gmail.com>
Signed-off-by: Tusharjamdade <tusharnjamdade@gmail.com>
|
Hi @illume, apologies for the multiple commits, I was running the formatter incorrectly earlier. I’ve now updated the code as per the review. Please take a look when you get a chance. |
illume
left a comment
There was a problem hiding this comment.
Thanks for the changes.
Could you please squash the commits?
Also, let us know if the open review comments have been addressed or not?
Allows configuring Prometheus using a full HTTP/HTTPS URL in addition to
namespace/service:port.This enables connecting clusters to external or shared Prometheus instances.
Keeps existing Kubernetes service-based configuration working.
Fixes #377.