-
Notifications
You must be signed in to change notification settings - Fork 6
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
Dashboard Builder Experiment #416
base: main
Are you sure you want to change the base?
Conversation
func BuildDashboards() (map[string]string, error) { | ||
operations, err := buildOperationsDashboard() | ||
if err != nil { | ||
return nil, err | ||
} | ||
return map[string]string{ | ||
"operations": string(operations), | ||
}, 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.
@K-Phoen I feel like this has the potential to be an interface from foundation-sdk that can be implemented so that we can consistently consume dashboards from upstream sources. IE, foundation sdk would have
type DasbhoardBuilder interface {
BuildDashboards() map[string]string, error
}
Then as a builder of dashboards, we'd implement the interface if we intend for it to be consumed. Does that make sense?
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.
I'd wait until we have more use-cases to try and define interfaces :)
Also: why not returning a dashboard builder in buildOperationsDashboard()
? To mimic to what we do with functions building & returning a panel and build stuff where and when it actually needs to be built.
It feels like this function will quickly get crowded by if err != nil
s otherwise 😅
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.
Another thought: what's the benefit of returning a map here? You seem to be ignoring that key later on
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.
I ignore it in the simplistic example, but the key is meant to be the filename. I wonder if I can skip that and just get the dashboard title 🤔
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.
Assigning a filename here seems a bit weird I think. What we're trying to represent feels closer to a collection of dashboard builders. Determining a filename for each of the resulting builders seems some other layer's responsibility to me :D
And it could also be done like this:
dashBuilders := dashboards.BuildDashboards()
if err != nil {
log.Fatalf("failed to build dashboard: %v", err)
}
for _, builder := range dashBuilders {
dash, err := builder.Build()
if err != nil {
log.Fatalf("failed to build dashboard: %v", err)
}
dashJson, _ := json.Marshal(dash)
filename := Slugify(dash.Title)+".json"
_ = os.WriteFile(filename, dashJson, 0644)
}
This way, building the dashboard collection doesn't need to bother itself with what will happen to the dashboards.
The objects themselves probably contain more than enough information to infer sensible filenames after the fact (title, uid, ...)
And bonus: whatever layer is responsible for writing/deploying the dashboards can choose the output format (json, yaml, ...) and maybe even add some kind of envelope (think grizzly for example)
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.
That all makes sense. I spoke with @\Duologic about how to import the dashboards in our internal monorepo and want to simplify what this PR is aiming to do. I'm going to focus on writing the dashboards out to json files and not consider trying to import the library and write them out.
The reason is we can vendor the dashboards into our monorepo with jb. IE, we can do
jb install https://github.com/grafana/cloudcost-exporter/cloudcost-exporter-dashboards@main
And then import the json files with jsonnet like we do with anything else.
One of the goals is to be able to consume the dashboards as if it they were a library so that they can be built in other contexts, such as internal repositories that manage dashboards. My proposal would be for there to be a way of getting all of the dashboards a project can export as a map where the key is the dashboard name, and the value is the string representation of the dashboard. This way a downstream application can do something like ```golang package main import ( "github.com/grafana/cloudcost-exporter/dashboards" ) func main() { for name, content := range dashboards { // write out content to ${name}.json } } ```
This moves over the config folder to `cloudcost-exporter-dashboards` so that we have a clear name when importing the dashboards with jsonnet. Updates `cloudcost-exporter-dashboards/main.go` to write out the dashboards to files if `--mode=file` is passed in when running the script. Adds two Makefile targets: - `grizzly-serve` for local development - `build-dashboards` to generate the files.
727dbaf
to
3bf9f3a
Compare
.github/workflows/tests.yml
Outdated
check-for-doc-changes: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Checkout | ||
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 #v4.2.2 | ||
- name: Install make | ||
run: sudo apt-get update && sudo apt-get install -y make | ||
shell: bash | ||
- name: Regenerate Helm Docs |
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.
Removing the helm test because it was duplicative of the test in helm.yml
workflow.
if *mode == "json" { | ||
fmt.Println(string(output)) | ||
continue | ||
} |
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.
I'm sure there's a way here to pass in something that implements the writer interface and we call w.Write, will revisit this if it's pressing.
Warning
This is a highly experimental PR that I'm mostly trying to throw some ideas out and get some feedback around the overall design.
One of the goals is to be able to consume the dashboards as if it they were a library so that they can be built in other contexts, such as internal repositories that manage dashboards.
My proposal would be for there to be a way of getting all of the dashboards a project can export as a map where the key is the dashboard name, and the value is the string representation of the dashboard. This way a downstream application can do something like
This PR refactors the current dashboard to
buildDashboard
methoddashboard.go
file that's responsible for returning a map of dashboardsconfig/grafana/main.go
has been updated to pull the dashboards for the dashboard package and print them out. So far Grizzly is able to handle a single dashboard, but I haven't tested with many.