monitoringdashboard: Add alertChart #2155
Conversation
justinsb
commented
Jun 26, 2024
•
justinsb
commented
- monitoringdashboard: Add alertChart
justinsb
force-pushed
the
monitoringdashboard_alertchart
branch
5 times, most recently
from
1d407dc
to
7c0d421
Compare
|
|
||
| package v1beta1 | ||
|
|
||
| type MonitoringAlertPolicyRef struct { |
There was a problem hiding this comment.
nit: we can put this resource reference to https://github.com/GoogleCloudPlatform/k8s-config-connector/tree/master/apis/refs/v1beta1 . Not a blocker
There was a problem hiding this comment.
I'll move it in a separate PR if that's OK. I was toying with the idea of grouping them by service though. Let's have a think in that other PR :-)
| @@ -25,6 +25,8 @@ output fields from GCP APIs are in `status.observedState.*` | |||
| * Added `spec.severity` field. | |||
|
|
|||
| * `MonitoringDashboard` | |||
| * Added `alertChart` widgets. | |||
| * Added `collapsibleGroup` widgets. | |||
There was a problem hiding this comment.
I don't see collapsibleGroup in this PR (guess that's due to some diffbase). Hope this won't cause conflict.
There was a problem hiding this comment.
Right - it's because we've already merged it, I just forgot to update the release notes!
| return nil | ||
| } | ||
| out := &pb.AlertChart{} | ||
| out.Name = in.AlertPolicyRef.External |
There was a problem hiding this comment.
Do we need to check the null pointer in.AlertPolicyRef here? (I hit a similar issue but use a different auto proto method)
There was a problem hiding this comment.
Technically no, because the field is required. But I agree with you, we should be checking it from a defensive point of view. Fixed!
| @@ -90,12 +90,12 @@ func (m *dashboardModel) AdapterForObject(ctx context.Context, kube client.Reade | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| projectID := projectRef.ProjectID | |||
| if projectID == "" { | |||
| if projectRef == nil || projectRef.ProjectID == "" { | |||
There was a problem hiding this comment.
I think we will never hit projectRef == nil(?) code , or maybe this infers you want to change the ResolveProject? Not a blocker, just curious about this change.
There was a problem hiding this comment.
True. I'm not sure why I did it. Reverting.
| if ref.Name == "" && ref.External == "" { | ||
| return nil, fmt.Errorf("must specify either name or external on reference") | ||
| } | ||
| if ref.Name != "" && ref.External != "" { |
There was a problem hiding this comment.
I saw the CRD already use oneOf.
But anyways, I think this entire function is helpful for all ResRef.
There was a problem hiding this comment.
Right. It doesn't cost us much, and then we don't need to rely on CRD validation.
BTW, I'm thinking we can greatly reduce code duplication here in future, maybe using AdapterForURL, or maybe just by adding a method to the Adapter called ResolveReference or something. But ... not a problem right now.
| return nil, err | ||
| } | ||
|
|
||
| ref = &krm.MonitoringAlertPolicyRef{ |
There was a problem hiding this comment.
oh, this alertPolicyRef is really complicated (wasn't expect it allows both projects/<project>/alertPolicies/<id> and alertPolicies/<id>)
I have a question about the AlertPolicy struct. Since the external has projects/<project>/alertPolicies/<id> I think the resource itself should have a spec.projectRef. But I don't see one
Do you know what happens?
There was a problem hiding this comment.
I think that's one of the "older" resources that uses the project-id annotation instead of projectRef.
I also think that's why project-id is listed here: https://cloud.google.com/config-connector/docs/reference/resource-docs/monitoring/monitoringalertpolicy#annotations
There was a problem hiding this comment.
(And ResolveProjectIDForObject tries projectRef first, then looks at the project-id annotation)
There was a problem hiding this comment.
That makes sense. Follow up discussion: since we validate and resolve the project from GCP server now, do you think we shall add the spec.projectRef for MonitoringAlertPolicy, and only support the previous project-id annotation for backward compatibility reasons? Not a blocker nor a necessary. But I think we have made a lot of improvements around project for the monitoring dashboard, why not make one more step further.
| if strings.HasSuffix(path, ".alertChart.alertPolicyRef.external") { | ||
| tokens := strings.Split(s, "/") | ||
| if len(tokens) > 2 { | ||
| switch tokens[len(tokens)-2] { |
There was a problem hiding this comment.
I guess the second to last token is always alertPolicies, right?
There was a problem hiding this comment.
If it's an alertPolicy, I believe so. It's normalization for tests, so it's not the end of the world if we're wrong.
(And similar to what you suggested the other day, I think we may be able to factor out some common logic here in future. The other option though is to build the replacements by looking at the http log, and share them with object normalization, rather than having to figure them out each time. TBD, I guess!)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yuwenma The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Implement as a ref
justinsb
force-pushed
the
monitoringdashboard_alertchart
branch
from
7c0d421
to
3dbe82a
Compare
|
Implemented the suggested nil-pointer check changes |
|
/lgtm Thanks! One more comment (no blocker) about if we want to add |
|
/hold cancel |
google-oss-prow
bot
merged commit 8ebebbd
into
GoogleCloudPlatform:master
