-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(observability-lib): builder to create independently grafana resources #915
Changes from all commits
12a6266
6afb436
236d7a4
52b24c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
package api | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/grafana/grafana-foundation-sdk/go/alerting" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestObjectMatchersEqual(t *testing.T) { | ||
t.Run("returns true if the two object matchers are equal", func(t *testing.T) { | ||
a := alerting.ObjectMatchers{{"team", "=", "chainlink"}} | ||
b := alerting.ObjectMatchers{{"team", "=", "chainlink"}} | ||
|
||
result := objectMatchersEqual(a, b) | ||
require.True(t, result) | ||
}) | ||
|
||
t.Run("returns true if the two object matchers with multiple matches are equal", func(t *testing.T) { | ||
a := alerting.ObjectMatchers{ | ||
{"team", "=", "chainlink"}, | ||
{"severity", "=", "critical"}, | ||
} | ||
b := alerting.ObjectMatchers{ | ||
{"severity", "=", "critical"}, | ||
{"team", "=", "chainlink"}, | ||
} | ||
|
||
result := objectMatchersEqual(a, b) | ||
require.True(t, result) | ||
}) | ||
|
||
t.Run("returns false if the two object matchers with multiple matches are different", func(t *testing.T) { | ||
a := alerting.ObjectMatchers{ | ||
{"team", "=", "chainlink"}, | ||
{"severity", "=", "critical"}, | ||
} | ||
b := alerting.ObjectMatchers{ | ||
{"severity", "=", "warning"}, | ||
{"team", "=", "chainlink"}, | ||
} | ||
|
||
result := objectMatchersEqual(a, b) | ||
require.False(t, result) | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,16 +29,23 @@ type BuilderOptions struct { | |
} | ||
|
||
func NewBuilder(options *BuilderOptions) *Builder { | ||
if options.TimeZone == "" { | ||
options.TimeZone = common.TimeZoneBrowser | ||
} | ||
builder := &Builder{} | ||
|
||
builder := &Builder{ | ||
dashboardBuilder: dashboard.NewDashboardBuilder(options.Name). | ||
Tags(options.Tags). | ||
Refresh(options.Refresh). | ||
Time(options.TimeFrom, options.TimeTo). | ||
Timezone(options.TimeZone), | ||
if options.Name != "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only creating a dashboard builder if we provide a name |
||
builder.dashboardBuilder = dashboard.NewDashboardBuilder(options.Name) | ||
if options.Tags != nil { | ||
builder.dashboardBuilder.Tags(options.Tags) | ||
} | ||
if options.Refresh != "" { | ||
builder.dashboardBuilder.Refresh(options.Refresh) | ||
} | ||
if options.TimeFrom != "" && options.TimeTo != "" { | ||
builder.dashboardBuilder.Time(options.TimeFrom, options.TimeTo) | ||
} | ||
if options.TimeZone == "" { | ||
options.TimeZone = common.TimeZoneBrowser | ||
} | ||
builder.dashboardBuilder.Timezone(options.TimeZone) | ||
} | ||
|
||
if options.AlertsTags != nil { | ||
|
@@ -104,33 +111,39 @@ func (b *Builder) AddNotificationPolicy(notificationPolicies ...*alerting.Notifi | |
b.notificationPoliciesBuilder = append(b.notificationPoliciesBuilder, notificationPolicies...) | ||
} | ||
|
||
func (b *Builder) Build() (*Dashboard, error) { | ||
db, errBuildDashboard := b.dashboardBuilder.Build() | ||
if errBuildDashboard != nil { | ||
return nil, errBuildDashboard | ||
} | ||
func (b *Builder) Build() (*Observability, error) { | ||
observability := Observability{} | ||
|
||
var alerts []alerting.Rule | ||
for _, alertBuilder := range b.alertsBuilder { | ||
alert, errBuildAlert := alertBuilder.Build() | ||
if errBuildAlert != nil { | ||
return nil, errBuildAlert | ||
if b.dashboardBuilder != nil { | ||
db, errBuildDashboard := b.dashboardBuilder.Build() | ||
if errBuildDashboard != nil { | ||
return nil, errBuildDashboard | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /nit, but when passing errors up the stack, it can be helpful for debugging to wrap the error with a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right I'll take care of this in future iterations |
||
} | ||
observability.Dashboard = &db | ||
|
||
// Add common tags to alerts | ||
if b.alertsTags != nil && len(b.alertsTags) > 0 { | ||
tags := maps.Clone(b.alertsTags) | ||
maps.Copy(tags, alert.Labels) | ||
var alerts []alerting.Rule | ||
for _, alertBuilder := range b.alertsBuilder { | ||
alert, errBuildAlert := alertBuilder.Build() | ||
if errBuildAlert != nil { | ||
return nil, errBuildAlert | ||
} | ||
|
||
alertBuildWithTags := alertBuilder.Labels(tags) | ||
alertWithTags, errBuildAlertWithTags := alertBuildWithTags.Build() | ||
if errBuildAlertWithTags != nil { | ||
return nil, errBuildAlertWithTags | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the difference between the tags and alert.Labels at this point? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. non-blocking, but if you could clarify in a future iteration please There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure |
||
|
||
alertBuildWithTags := alertBuilder.Labels(tags) | ||
alertWithTags, errBuildAlertWithTags := alertBuildWithTags.Build() | ||
if errBuildAlertWithTags != nil { | ||
return nil, errBuildAlertWithTags | ||
} | ||
alerts = append(alerts, alertWithTags) | ||
} else { | ||
alerts = append(alerts, alert) | ||
} | ||
alerts = append(alerts, alertWithTags) | ||
} else { | ||
alerts = append(alerts, alert) | ||
} | ||
observability.Alerts = alerts | ||
} | ||
|
||
var contactPoints []alerting.ContactPoint | ||
|
@@ -141,6 +154,7 @@ func (b *Builder) Build() (*Dashboard, error) { | |
} | ||
contactPoints = append(contactPoints, contactPoint) | ||
} | ||
observability.ContactPoints = contactPoints | ||
|
||
var notificationPolicies []alerting.NotificationPolicy | ||
for _, notificationPolicyBuilder := range b.notificationPoliciesBuilder { | ||
|
@@ -150,11 +164,7 @@ func (b *Builder) Build() (*Dashboard, error) { | |
} | ||
notificationPolicies = append(notificationPolicies, notificationPolicy) | ||
} | ||
observability.NotificationPolicies = notificationPolicies | ||
|
||
return &Dashboard{ | ||
Dashboard: &db, | ||
Alerts: alerts, | ||
ContactPoints: contactPoints, | ||
NotificationPolicies: notificationPolicies, | ||
}, nil | ||
return &observability, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the tests are good 👍 - a comment saying that this is an
unordered equal
should suffice for the readerThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree Im adding the comment