refactor(monitoring): migrate MonitoringDashboard controller to Google REST client - #13574
Conversation
…e REST client Migrate MonitoringDashboard direct controller from the Cloud Monitoring v1 gRPC client to google.golang.org/api/monitoring/v1 (REST client). Using the Google REST client aligns request/response payloads with MockGCP, natively using string enums instead of integer enum encodings. This enables 100% golden log alignment across all MonitoringDashboard fixtures without requiring custom enum mappers or normalizations in golden_alignment_test.go.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
gemmahou
left a comment
gemmahou
left a comment
There was a problem hiding this comment.
/lgtm
Two nit comments...
| @@ -235,11 +234,6 @@ func (d *dashboardDefaulter) visitPieChart(obj *pb.PieChart) { | |||
| } | |||
|
|
|||
| func (d *dashboardDefaulter) visitTimeSeriesTable(obj *pb.TimeSeriesTable) { | |||
There was a problem hiding this comment.
nit: remove this empty function
| func computeEtag(obj proto.Message) string { | ||
| b, err := proto.Marshal(obj) | ||
| clone := proto.Clone(obj) | ||
| if dashboard, ok := clone.(*pb.Dashboard); ok { |
There was a problem hiding this comment.
nit: I feel this function is to generate unique Etag for objects, I'm curious why do we need this if logic? The dashboard doesn't seem to be used here.
There was a problem hiding this comment.
IIRC the controller is using etag for reconciliation, we should revise that but it's out of scope for the change. I'll let the agent take a look though and reply as well.
There was a problem hiding this comment.
Two details explain why this block is there:
- In Go,
dashboardis a typed pointer to the clone message (clone := proto.Clone(obj)), so settingdashboard.Name = ""anddashboard.Etag = ""mutatesclonein place beforeproto.Marshal(clone)is called. - We zero out
EtagandNameon the clone so the hash is deterministically computed from the dashboard specification alone:- On updates, incoming objects already have an existing
etagset, which would otherwise pollute the computed hash. - MockGCP generates random UUIDs in
Name, so clearingNameensures the Etag does not change across runs or mock recreations.
- On updates, incoming objects already have an existing
I have added an explanatory doc comment to computeEtag and removed the empty visitTimeSeriesTable helper function as well!
|
New changes are detected. LGTM label has been removed. |
This branch has not been deployed
2 participants
What this PR does / why we need it:
Prerequisite for #12458.
Migrates the
MonitoringDashboarddirect controller fromcloud.google.com/go/monitoring/dashboard/apiv1(gRPC client) togoogle.golang.org/api/monitoring/v1(Google REST client).Using the Google REST client aligns request/response payloads with MockGCP, natively using string enums instead of integer enum encodings (
%24alt=json%3Benum-encoding%3Dint). This achieves 100% golden log alignment across allMonitoringDashboardtest fixtures without requiring enum mappings or normalizations ingolden_alignment_test.go.Additionally:
mockgcp/generated/mockgcp/monitoring/dashboard/v1(from feat(mockgcp): add google.monitoring.dashboard.v1 proto definitions #13471).dashboardFilters.valueTypeintests/e2e/normalize.go.minAlignmentPeriod: "0s"defaulter in MockGCP to match real GCP behavior.Which issue(s) this PR fixes:
Prerequisite for #7090 / #12458
Does this PR introduce a user-facing change?