Skip to content
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

Merged
merged 11 commits into from
Sep 27, 2024

Conversation

tlindsay
Copy link
Member

@tlindsay tlindsay commented Sep 16, 2024

@tlindsay tlindsay force-pushed the pl/custom-dashboards branch from 05304a7 to 13c5804 Compare September 17, 2024 20:56
@tlindsay tlindsay marked this pull request as draft September 18, 2024 19:00
@tlindsay tlindsay force-pushed the pl/custom-dashboards branch from 61c01aa to 5f00e27 Compare September 25, 2024 16:36
@tlindsay tlindsay force-pushed the pl/custom-dashboards branch from 9e58597 to a5d0b52 Compare September 25, 2024 18:50
@tlindsay tlindsay force-pushed the pl/custom-dashboards branch from a5d0b52 to 5f8f4f8 Compare September 25, 2024 18:51
fastly/observability_custom_dashboard.go Show resolved Hide resolved
fastly/observability_custom_dashboard.go Show resolved Hide resolved
Visualization DashboardVisualization `json:"visualization"`
}

type DashboardSourceType string
Copy link
Member Author

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!

Copy link

@ydnar ydnar Sep 27, 2024

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.

Copy link

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.

@tlindsay tlindsay marked this pull request as ready for review September 25, 2024 19:12
@kpfleming kpfleming changed the title pl/custom dashboards feat(custom_dashboards): Add support for Custom Dashboards APIs. Sep 25, 2024
@tlindsay tlindsay requested a review from a team September 25, 2024 19:19
Copy link
Contributor

@kpfleming kpfleming left a 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.

fastly/observability_custom_dashboard.go Show resolved Hide resolved
fastly/observability_custom_dashboard.go Show resolved Hide resolved
fastly/observability_custom_dashboard.go Outdated Show resolved Hide resolved
@tlindsay tlindsay requested a review from kpfleming September 25, 2024 21:59
Copy link

@ydnar ydnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor design notes

@kpfleming kpfleming merged commit 4034f9c into main Sep 27, 2024
4 checks passed
@kpfleming kpfleming deleted the pl/custom-dashboards branch September 27, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants