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

Dashboard Builder Experiment #416

Merged
merged 13 commits into from
Feb 3, 2025
Merged

Conversation

Pokom
Copy link
Contributor

@Pokom Pokom commented Jan 28, 2025

The previous PR(#405) migrated an internal operational dashboard to being generated with the Go Foundation SDK. You could render the dashboard locally with grizzly and generate it, but there was nothing in place to generate the dashboards.

This PR introduces

  • cmd/dashboards which is responsible for building the dashboards with the option to write them out
  • CI step to detect drift on the dashboard and fail the build if it is
  • Makefile targets to run grizzly and generate dashboards
  • Initial commit of the operational dashboard's json blob

@Pokom Pokom requested a review from K-Phoen January 28, 2025 21:19
Comment on lines 3 to 11
func BuildDashboards() (map[string]string, error) {
operations, err := buildOperationsDashboard()
if err != nil {
return nil, err
}
return map[string]string{
"operations": string(operations),
}, nil
}
Copy link
Contributor Author

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?

Copy link
Member

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 != nils otherwise 😅

Copy link
Member

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

Copy link
Contributor Author

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 🤔

Copy link
Member

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)

Copy link
Contributor Author

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.

Pokom added 4 commits January 30, 2025 15:56
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.
@Pokom Pokom force-pushed the exp-dashboard-builder branch from 727dbaf to 3bf9f3a Compare January 30, 2025 20:58
@Pokom Pokom marked this pull request as ready for review January 30, 2025 21:23
@Pokom Pokom requested a review from a team as a code owner January 30, 2025 21:23
.github/workflows/tests.yml Outdated Show resolved Hide resolved
Comment on lines 38 to 41
if *mode == "json" {
fmt.Println(string(output))
continue
}
Copy link
Contributor Author

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.

Comment on lines -30 to 35
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
- name: Check for Dashboards Drift
run: |
make helm > /dev/null # we don't actually need to output, just the results
make build-dashboards > /dev/null
if ! git diff --exit-code; then
echo "Helm docs are out of date. Please run 'make -C deploy/helm docs' and commit the changes."
echo "Dashboards are out of sync. Please run 'make build-dashboards' and commit the changes."
exit 1
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#417 included changes in tests.yml that were duplicated so I could validate the file. GitHub Actions won't pick up new actions in PR's

@Pokom Pokom force-pushed the exp-dashboard-builder branch from 87728e9 to f861c9c Compare February 3, 2025 14:51
return strings.ReplaceAll(strings.ToLower(s), " ", "-")
}

func createFile(outputDir, name string, data []byte) error {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: https://pkg.go.dev/os#WriteFile could probably be used instead :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I always forget the many ways of creating a file in Go. Let me refactor it to use that since it's simpler

@Pokom Pokom merged commit bc5dea7 into grafana:main Feb 3, 2025
2 checks passed
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.

2 participants