Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds HTTPS support for the metric dumper by refactoring URL handling throughout the codebase. The changes enable the system to work with both HTTP and HTTPS Prometheus endpoints.
Key changes:
- Introduced a new
URL2Nameutility function to convert URLs to filesystem-safe names - Modified URL construction to include protocol prefixes (http:// or https://)
- Updated the
makeURLfunction to handle URLs that already contain protocol prefixes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/utils/utils.go | Added URL2Name utility function to sanitize URLs for filesystem usage by removing protocols and replacing special characters |
| collector/prometheus.go | Updated URL construction across multiple functions to properly handle protocol prefixes, enabling HTTPS support for Prometheus endpoints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
The function URL2Name is missing documentation. Since this is an exported function (starts with uppercase), it should have a doc comment explaining its purpose, parameters, and return value. For example:
// URL2Name converts a URL to a filesystem-safe name by removing the protocol
// prefix (http:// or https://) and replacing special characters (/ and :) with underscores.
func URL2Name(url string) string {| // URL2Name converts a URL to a filesystem-safe name by removing the protocol | |
| // prefix (http:// or https://) and replacing special characters (/ and :) with underscores. |
| } else { | ||
| for _, prom := range topo.Monitors { | ||
| monitors = append(monitors, fmt.Sprintf("%s:%d", prom.Host(), prom.MainPort())) | ||
| // todo: check is TLS enabled in tiup deployed prometheus |
There was a problem hiding this comment.
[nitpick] The TODO comment mentions checking if TLS is enabled for tiup deployed prometheus, but there's no mechanism to actually detect or handle this. The code currently hardcodes http:// as the protocol. Consider implementing the TLS detection or document the expected behavior when TLS is enabled (e.g., users should manually configure endpoints with https:// via AttrKeyPromEndpoint).
| // todo: check is TLS enabled in tiup deployed prometheus | |
| // TODO: If Prometheus is deployed with TLS enabled, users must manually configure endpoints with `https://` via `AttrKeyPromEndpoint`. |
|
LGTM |
2 participants
Also fixs empty result when prometheus caontains /