Add xray openMetrics#183
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds JFrog Xray OpenMetrics collection support while refactoring existing OpenMetrics code for better reusability. It introduces a new --xray.scrape-uri flag and xray optional metric, along with a modular processOpenMetrics() helper function that extracts shared processing logic. The changes also implement metric prefixing for process_* metrics to prevent collisions with Prometheus internal metrics.
Key changes:
- Refactored OpenMetrics processing into a reusable
processOpenMetrics()helper function - Added Xray OpenMetrics support with new configuration flag and optional metric
- Implemented metric name prefixing to avoid conflicts with internal Prometheus metrics
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| config/config.go | Added xray optional metric, --xray.scrape-uri flag, and XrayScrapeURI configuration field with validation |
| config/config_test.go | Updated test to include xray in the expected optional metrics list |
| collector/openMetricsHelper.go | New file containing extracted sanitizeOpenMetrics() and processOpenMetrics() helper functions |
| collector/openMetrics.go | Refactored to use processOpenMetrics() helper and added exportXrayOpenMetrics() function |
| collector/collector.go | Integrated Xray OpenMetrics export into the main collection flow |
| artifactory/openmetrics.go | Added FetchXrayOpenMetrics() method with URI swapping mechanism |
| artifactory/client.go | Added XrayURI field to Client struct |
| README.md | Updated documentation to describe new --xray.scrape-uri flag and xray optional metric |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fName := family.GetName() | ||
| fHelp := family.GetHelp() | ||
| if strings.HasPrefix(fName, "process_") { // avoid conflict with promethues internal metrics | ||
| fName = source + fName |
There was a problem hiding this comment.
The prefix concatenation for process_* metrics results in names like artifactoryprocess_* or xrayprocess_*, missing an underscore separator. This should be fName = source + "_" + fName to produce properly formatted metric names like artifactory_process_* and xray_process_*.
| fName = source + fName | |
| fName = source + "_" + fName |
| func (e *Exporter) exportXrayOpenMetrics(ch chan<- prometheus.Metric) error { | ||
| openMetrics, err := e.client.FetchXrayOpenMetrics() | ||
| if err != nil { | ||
| e.logger.Error("There was an issue when try to fetch openMetrics") |
There was a problem hiding this comment.
Grammatical error in error message: "when try to fetch" should be "when trying to fetch". This matches the pattern used elsewhere in the codebase (e.g., artifactory/client.go:104, artifactory/federation.go:86).
|
@maitreyaatish Thanks for the contribution. Please check copilot comments and address the relevant ones |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| } else if optMetrics.Xray { |
There was a problem hiding this comment.
The validation logic has a flaw: xrayScrapeURI is defined with a default value of "http://localhost:8081/xray" at line 21, which means it will never be an empty string. Therefore, the else condition at line 174 checking if optMetrics.Xray is true will never execute. This prevents the proper validation that requires users to explicitly set the Xray scrape URI when enabling the xray optional metric. Consider removing the default value or adjusting the validation logic to allow the default only when the xray metric is not explicitly enabled.
| } else if optMetrics.Xray { | |
| } | |
| if optMetrics.Xray && (*xrayScrapeURI == "" || *xrayScrapeURI == "http://localhost:8081/xray") { |
| func (e *Exporter) exportXrayOpenMetrics(ch chan<- prometheus.Metric) error { | ||
| openMetrics, err := e.client.FetchXrayOpenMetrics() | ||
| if err != nil { | ||
| e.logger.Error("There was an issue when try to fetch openMetrics") |
There was a problem hiding this comment.
Grammar error: "when try to fetch" should be "when trying to fetch". This error message appears in both exportOpenMetrics and exportXrayOpenMetrics functions.
| if err != nil { | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
There's an unnecessary blank line between the closing brace and the next statement. Remove the extra blank line at line 217 for consistency with the rest of the codebase.
| if err.(*APIError).status == 404 { | ||
| return openMetrics, nil |
There was a problem hiding this comment.
Unsafe type assertion: if the error is not of type *APIError, this will cause a panic. Use a type assertion with a safety check instead, such as: if apiErr, ok := err.(*APIError); ok && apiErr.status == 404. This is particularly important because FetchHTTP can return other error types like UnmarshalError or network errors.
Adds JFrog Xray OpenMetrics collection while refactoring existing OpenMetrics code for better reusability.