PO to GMP Migration Tool: Podmonitor Limits, ScrapeClass, Metadata. FilterRunning Migration - #1989
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 adds support for converting limits, node metadata attachment, and filter-running configurations from Prometheus PodMonitor resources to GMP PodMonitoring and ClusterPodMonitoring resources, alongside adding relevant unit tests and warnings for unsupported fields. The review feedback points out a concurrency/mutation risk where the metadata slice is modified in-place via pointer dereference. To prevent unexpected side effects on shared or cached configurations, it is recommended to copy the slice before appending new elements.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements conversion logic for limits, node metadata attachment, and filter-running configurations from Prometheus PodMonitors to GMP PodMonitoring and ClusterPodMonitoring resources, along with corresponding test coverage and warnings for unsupported fields. The review feedback suggests improving the warning messages for endpoint-level filterRunning settings to clarify that the tool automatically applies the configuration globally and to ensure the correct resource type is referenced in the ClusterPodMonitoring converter.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for mapping PodMonitor limit settings to GMP ScrapeLimits and handles several unsupported fields (such as followRedirects, enableHttp2, scrapeClassName, and scrapeProtocols) by logging warnings. It also adds conversion logic for AttachMetadata.Node and resource-level FilterRunning settings. The review feedback suggests avoiding taking the address of block-local variables when updating metadata, and refining the FilterRunning warning logic to only trigger when there is an actual conflict between endpoints.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for converting PodMonitor limit settings, node metadata attachment, and filter-running configurations to GMP ScrapeLimits and PodMonitoring/ClusterPodMonitoring specs, along with warnings for unsupported fields. The reviewer recommends extracting the duplicated spec-level field processing logic from both convertToPodMonitoring and convertToClusterPodMonitoring into a single shared helper function in helpers.go to reduce code duplication and improve maintainability.
43c6120 to
cbeae05
Compare
cbeae05 to
99b9190
Compare
feba832 to
034c91a
Compare
ee7f76d to
3273c74
Compare
| warnUnsupportedSpecFields(logger, pm.Spec) | ||
| // TODO(M2): Resolve and merge ScrapeClass configurations from Prometheus CR if scrapeClassName is specified. | ||
| if pm.Spec.ScrapeClassName != nil && *pm.Spec.ScrapeClassName != "" { | ||
| logger.Warn(fmt.Sprintf("ScrapeClass %q was not found in the inputs. The 'scrapeClassName' field has been dropped and inherited settings will be lost.", *pm.Spec.ScrapeClassName)) |
There was a problem hiding this comment.
The warning claims was not found in the inputs, implying a lookup was attempted. Saying is not yet supported or could not be resolved would be clearer.
There was a problem hiding this comment.
Plan is to add support in M2, but I could temporarily change the logs to `is not yet supported``
|
|
||
| // Constants representing the supported ScrapeProtocol enum values defined in upstream Prometheus Operator. | ||
| const ( | ||
| scrapeProtocolOpenMetricsText001 = pomonitoringv1.ScrapeProtocol("OpenMetricsText0.0.1") |
There was a problem hiding this comment.
Unused constant scrapeProtocolOpenMetricsText001 (along with scrapeProtocolPrometheusText004 on line 49). Consider removing them if they aren't needed.
There was a problem hiding this comment.
Defined them in case they are needed for the future and/or for test cases -- but can remove them
| @@ -1435,6 +1884,16 @@ func TestPodMonitorConversion(t *testing.T) { | |||
| t.Fatalf("Convert failed: %v", err) | |||
There was a problem hiding this comment.
wantWarnings only asserts positive matches. Adding a dontWantWarnings []string field to the test table would allow pinning negative cases (e.g. verifying no conflict warning is emitted when all endpoints set filterRunning: false).
2 participants
This change completes the missing configuration fields in the PodMonitor migration path to GKE Managed Prometheus (PodMonitoring and ClusterPodMonitoring).
Key changes:
attachMetadata.nodetotargetLabels.metadata: ["node"].sampleLimit,labelLimit,labelNameLengthLimit, andlabelValueLengthLimitto GMP'sScrapeLimits.filterRunningconfigurations to the resource level.scrapeClassNameis configured, noting that inherited settings will be lost. Added a TODO code comments to resolve and mergeScrapeClasssettings from the Prometheus CR once that migration pipeline is implemented.