PO to GMP Migration Tool: Refactor Common Conversion Functions by karthunni · Pull Request #1990 · GoogleCloudPlatform/prometheus-engine

karthunni · 2026-07-21T18:19:12Z

Refactoring conversion functions to reduce code duplication and simplify implementation for impending ServiceMonitor migration logic.

gemini-code-assist

Code Review

This pull request refactors the migration helpers and PodMonitor converter by extracting common logic into shared helper functions and introducing a commonMonitorSpec struct. Feedback is provided to address a bug in resolveFilterRunning where explicit filterRunning: true configurations are silently dropped, and to add defensive nil checks for the spec parameter in buildPodMonitoring and buildClusterPodMonitoring to prevent potential nil pointer dereferences.

karthunni · 2026-07-23T16:13:38Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the migration helpers and PodMonitor converter to reduce code duplication and improve maintainability. It introduces a shared commonMonitorSpec struct and extracts several helper functions for resolving scrape intervals, timeouts, proxy URLs, authentication, TLS settings, and unsupported fields. These helpers are then utilized to consolidate the conversion logic for both PodMonitoring and ClusterPodMonitoring resources. I have no feedback to provide as the changes are clean and well-structured.

karthunni · 2026-07-23T18:33:28Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the migration logic for PodMonitor resources by extracting common conversion logic, validation, and warning routines into helper functions within pkg/migrate/helpers.go. It also introduces a commonMonitorSpec struct to share configurations between namespaced and cluster-scoped resources, significantly reducing code duplication. The reviewer feedback suggests adding a nil check for the gmpEp parameter in the new applyAuthAndTLS helper function to prevent potential nil pointer dereferences.

karthunni · 2026-07-23T18:43:09Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors the PodMonitor migration logic by modularizing helper functions into helpers.go and introducing a shared commonMonitorSpec struct to streamline the conversion of both PodMonitoring and ClusterPodMonitoring resources. The review feedback focuses on improving resilience during migration; specifically, it suggests refactoring determineNamespaceScoping, resolveScrapeIntervalAndTimeout, and convertProxyURL to log warnings and fall back to safe defaults or placeholders instead of returning fatal errors when encountering non-fatal configuration issues.

dashpole · 2026-07-30T00:10:27Z

+		return nil, err
 	}
 	if len(mergedSelector.MatchLabels) == 0 && len(mergedSelector.MatchExpressions) == 0 {
 		logger.Warn("Resulting PodMonitoring selector is empty. It will select and scrape all pods in this namespace. Verify if this is intended.")


The empty selector check was moved into convertToMonitorSpec, so converting a cluster-scoped monitor now logs Resulting PodMonitoring selector is empty... in this namespace instead of ClusterPodMonitoring across all namespaces. Consider passing the resource kind down or moving the warning to the kind-specific builder.

dashpole · 2026-07-30T00:10:27Z

 	return limits
 }
+
+// toStrictUnstructured converts a struct to a strictly JSON-compatible unstructured map.


The comment states JSON is used for unsupported Go primitives like uint64, but ToUnstructured does support uint64 (the real issue is unstructured.DeepCopy panicking on uint64). Also, unmarshaling numbers into float64 loses integer precision above 2^53 for large ScrapeLimits. Consider normalizing limits to int64 or using json.NewDecoder with UseNumber().

dashpole · 2026-07-30T00:10:27Z

+}
+
+// resolveAttachMetadata appends "node" to metadata if attachMetadata.node is enabled.
+func resolveAttachMetadata(attachMetadata *pomonitoringv1.AttachMetadata, baseMetadata *[]string) *[]string {


Because resolveAttachMetadata runs before unionMetadata, setting attachMetadata.node: true returns a non-nil slice (node), which causes unionMetadata to pull in all default metadata labels (container, pod, etc.). It would be helpful to add a comment explaining this tradeoff.

dashpole · 2026-07-30T00:10:27Z

+	}, nil
+}
+
+func (c *PodMonitorConverter) convertToPodMonitoring(pm *pomonitoringv1.PodMonitor, logger *slog.Logger, cache *ResourceCache) (*unstructured.Unstructured, []*unstructured.Unstructured, error) {


convertToPodMonitoring and convertToClusterPodMonitoring are structurally identical except for their defaults slice and builder call. Consider collapsing them into a single parameterized helper.

dashpole · 2026-07-30T00:10:27Z

 }

+// applyAuthAndTLS converts credentials and TLS settings for a generic endpoint.
+func (c *conversionContext) applyAuthAndTLS(


applyAuthAndTLS accepts i int to format endpoint [%d], but the caller at podmonitor.go:155 wraps the error again, producing redundant error context. Consider dropping i from applyAuthAndTLS and wrapping errors at the call site.

dashpole · 2026-07-30T00:10:27Z

@@ -77,21 +81,21 @@ type relabelingData struct {
 	rewrittenSources []string


PreScrapeRelabelingResult and ExtractedPreScrapeRules were exported while parseAndCleanNamespaces was un-exported. Since neither struct is used outside pkg/migrate, consider keeping them un-exported.

dashpole · 2026-07-30T00:10:27Z

+func resolveAttachMetadata(attachMetadata *pomonitoringv1.AttachMetadata, baseMetadata *[]string) *[]string {
+	if attachMetadata != nil && attachMetadata.Node != nil && *attachMetadata.Node {
+		if baseMetadata == nil {
+			return &[]string{"node"}


Quick nit: resolveAttachMetadata hardcodes "node" directly instead of using the labelNode constant defined earlier in the file.

dashpole · 2026-07-30T00:10:27Z

+}
+
+// combineAndConvertRelabelings combines promoted pre-scrape rules and converts metricRelabelings.
+func combineAndConvertRelabelings(logger *slog.Logger, promoted []monitoringv1.RelabelingRule, configs []pomonitoringv1.RelabelConfig) ([]monitoringv1.RelabelingRule, error) {


combineAndConvertRelabelings returns an error, but convertMetricRelabelings always returns nil error. Consider dropping the error return from both signatures.

dashpole · 2026-07-30T00:10:27Z

 }

+// determineNamespaceScoping resolves the target namespaces from a NamespaceSelector.
+func determineNamespaceScoping(nsSel pomonitoringv1.NamespaceSelector, defaultNS string) ([]string, bool, error) {


The newly extracted helpers (determineNamespaceScoping, resolveScrapeIntervalAndTimeout, convertProxyURL, resolveFilterRunning, resolveAttachMetadata, toStrictUnstructured) currently lack direct unit tests in helpers_test.go. Adding table-driven unit tests would ensure boundary conditions and error cases are thoroughly covered.

karthunni self-assigned this Jul 21, 2026

karthunni mentioned this pull request Jul 21, 2026

PO to GMP Migration Tool: Podmonitor Limits, ScrapeClass, Metadata. FilterRunning Migration #1989

Open

karthunni changed the title ~~PO to GMP Migration Tool: Refactor Conversion Functions~~ PO to GMP Migration Tool: Refactor Common Conversion Functions Jul 21, 2026

gemini-code-assist Bot reviewed Jul 21, 2026

View reviewed changes

Comment thread pkg/migrate/helpers.go

Comment thread pkg/migrate/helpers.go

Comment thread pkg/migrate/helpers.go

karthunni force-pushed the karthunni/po-migrate-refactor branch 2 times, most recently from 3b27e3b to 973c9b2 Compare July 21, 2026 18:35

karthunni force-pushed the karthunni/po-migrate-refactor branch from 973c9b2 to e8cd8d0 Compare July 23, 2026 15:14

gemini-code-assist Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread pkg/migrate/helpers.go

gemini-code-assist Bot reviewed Jul 23, 2026

View reviewed changes

Comment thread pkg/migrate/helpers.go

Comment thread pkg/migrate/podmonitor.go

Comment thread pkg/migrate/helpers.go

Comment thread pkg/migrate/podmonitor.go

Comment thread pkg/migrate/helpers.go

Comment thread pkg/migrate/podmonitor.go

karthunni force-pushed the karthunni/po-migrate-refactor branch from dd4645d to e7f1688 Compare July 27, 2026 19:05

karthunni force-pushed the karthunni/po-migrate-refactor branch from e7f1688 to 578df0a Compare July 28, 2026 15:16

dashpole self-assigned this Jul 29, 2026

karthunni marked this pull request as ready for review July 29, 2026 20:45

karthunni added 8 commits July 29, 2026 20:54

refactor: extract spec logic and builders

056275c

refactor: create helper functions

a16a4ce

fix: add nil check on spec

fb85593

refactor: create helpers for endpoint specific logic

19ed09d

fix: resolve lint errors

8ad6d67

refactor: namespace and relabeling refactor

e0efa98

fix: resolve lint errors and nil pointer

71d2751

fix: rebase changes

643c79c

karthunni force-pushed the karthunni/po-migrate-refactor branch from f690691 to 643c79c Compare July 29, 2026 20:55

karthunni requested a review from bernot-dev July 29, 2026 21:54

dashpole reviewed Jul 30, 2026

View reviewed changes

		@@ -77,21 +81,21 @@ type relabelingData struct {
		rewrittenSources []string

Uh oh!

Conversation

karthunni commented Jul 21, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

karthunni commented Jul 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

karthunni commented Jul 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

karthunni commented Jul 23, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants