-
Notifications
You must be signed in to change notification settings - Fork 126
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(custom_dashboards): Add support for Custom Dashboards APIs. #546
Conversation
05304a7
to
13c5804
Compare
61c01aa
to
5f00e27
Compare
9e58597
to
a5d0b52
Compare
a5d0b52
to
5f8f4f8
Compare
Visualization DashboardVisualization `json:"visualization"` | ||
} | ||
|
||
type DashboardSourceType string |
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.
Naming types/constants is really hard. I'm not sure how to balance user-ergonomics with collision-avoidance since all of our API wrappers live in the same package/namespace.
Advice/input is welcomed!
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.
If you have generally acyclic type dependencies, it's a common pattern to split types/functions into a sub-package, eg .../dashboard
which acts as a namespace.
The caller side looks like dashboard.Observability
, dashboard.Type
, dashboard.Options
etc.
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.
Go type aliases can be used to migrate gradually without breaking backwards compatibility.
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.
Overall this looks pretty good, but I've asked for one relatively small change.
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.
minor design notes
DI-1346 Implements client for Observability Custom Dashboards API