feat(observability-lib): builder to create independently grafana resources #915
Conversation
| Refresh(options.Refresh). | ||
| Time(options.TimeFrom, options.TimeTo). | ||
| Timezone(options.TimeZone), | ||
| if options.Name != "" { |
There was a problem hiding this comment.
Only creating a dashboard builder if we provide a name
| builder := grafana.NewBuilder(&grafana.BuilderOptions{}) | ||
| builder.AddContactPoint(grafana.NewContactPoint(&grafana.ContactPointOptions{ | ||
| Name: "slack", | ||
| Type: "slack", | ||
| })) | ||
|
|
||
| o, err := builder.Build() | ||
| if err != nil { | ||
| t.Errorf("Error during build: %v", err) | ||
| } | ||
|
|
||
| require.Empty(t, o.Dashboard) | ||
| require.Empty(t, o.Alerts) | ||
| require.NotEmpty(t, o.ContactPoints) | ||
| require.Empty(t, o.NotificationPolicies) |
There was a problem hiding this comment.
We can now use the library to create a single contact point
| builder := grafana.NewBuilder(&grafana.BuilderOptions{}) | ||
| builder.AddNotificationPolicy(grafana.NewNotificationPolicy(&grafana.NotificationPolicyOptions{ | ||
| Receiver: "slack", | ||
| GroupBy: []string{"grafana_folder", "alertname"}, | ||
| ObjectMatchers: []alerting.ObjectMatcher{ | ||
| {"team", "=", "chainlink"}, | ||
| }, | ||
| })) | ||
|
|
||
| o, err := builder.Build() | ||
| if err != nil { | ||
| t.Errorf("Error during build: %v", err) | ||
| } | ||
|
|
||
| require.Empty(t, o.Dashboard) | ||
| require.Empty(t, o.Alerts) | ||
| require.Empty(t, o.ContactPoints) | ||
| require.NotEmpty(t, o.NotificationPolicies) | ||
| }) |
There was a problem hiding this comment.
We can now use the library to create a single notification policy
| type Dashboard struct { | ||
| type Observability struct { | ||
| Dashboard *dashboard.Dashboard | ||
| Alerts []alerting.Rule | ||
| ContactPoints []alerting.ContactPoint | ||
| NotificationPolicies []alerting.NotificationPolicy | ||
| } |
There was a problem hiding this comment.
renaming as the struct is holding all type of grafana resources not only dashboard anymore
| for j := range b { | ||
| if reflect.DeepEqual(a[i], b[j]) { | ||
| foundMatch = true | ||
| break |
There was a problem hiding this comment.
this doesn't look like an exact match - could you leave a comment on the method explaining when a and b should be equal to each other?
There was a problem hiding this comment.
associated tests here : https://github.com/smartcontractkit/chainlink-common/pull/915/files#diff-5b4adecb60cf2e79cc22806a969468909ea9f8bfa52bd464a0add735ef682cbeR10-R46 it should explain what is expected
There was a problem hiding this comment.
yes, the tests are good 👍 - a comment saying that this is an unordered equal should suffice for the reader
There was a problem hiding this comment.
agree Im adding the comment
| if b.dashboardBuilder != nil { | ||
| db, errBuildDashboard := b.dashboardBuilder.Build() | ||
| if errBuildDashboard != nil { | ||
| return nil, errBuildDashboard |
There was a problem hiding this comment.
/nit, but when passing errors up the stack, it can be helpful for debugging to wrap the error with a fmt.Errorf with a description of what the consumer observed with that error, i.e.
return nil, fmt.Errorf("failure in building dashboard: %w, errBuildDashboard")
There was a problem hiding this comment.
right I'll take care of this in future iterations
| // Add common tags to alerts | ||
| if b.alertsTags != nil && len(b.alertsTags) > 0 { | ||
| tags := maps.Clone(b.alertsTags) | ||
| maps.Copy(tags, alert.Labels) |
There was a problem hiding this comment.
what is the difference between the tags and alert.Labels at this point?
There was a problem hiding this comment.
Its the same I use tags as a name but grafana calls them Labels I could rename vars
There was a problem hiding this comment.
non-blocking, but if you could clarify in a future iteration please
Builder refactor to allow creating contact points and notification policies without creating a dashboard