Skip to content
This repository was archived by the owner on Mar 6, 2025. It is now read-only.

Conversation

@nodehatch
Copy link
Contributor

Description

What does this PR do?
Adds opa server dashboard

PR checklist for examples

Check these items before merging the PR.

  • Each example contains a README file.
  • README documents all steps and variables required to run the example. The simplest way to confirm this is to follow the instructions on a clean machine.
  • README file contains the links to the official documentation with relevant configuration and to the metrics published if that is available.
  • Used images of apps and collectors have to be pinned, "latest" should never be used.
  • Evidences with screenshots provided from the reviewer.
  • Includes a file called metrics.csv with metrics produced by the example.
    The file should have these 5 headings: Name, Description, Unit, DataType, Attributes.
    Description is not provided for all metrics, so it may be blank. Attributes are also not always provided. When there are multiple attributes on a metric reord they should be space separated.
  • Dashboard provided with example and put to the folder "dashboard" inside the example folder.
  • At least one screenshot of the proposed dashboard is included in this PR. If you're proposing substantive changes to queries or new queries then please ensure your screenshot shows relevant data.
  • All chart names in the dashboard are in Title case.
  • All query names in the dashboard are lower case letters, beginning the first query of each chart as "a" and proceeding alphabetically ... "b", "c", etcetera.

@nodehatch nodehatch self-assigned this Sep 18, 2023
@nodehatch
Copy link
Contributor Author

app lightstep com_dev_bcronin_dashboard_open-policy-agent_1H80WpJt_time_window=minutes_60

Copy link
Contributor

@nslaughter nslaughter left a comment

Choose a reason for hiding this comment

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

Changes

Please rename the folder to openpolicyagent. That's my interpretation of what your average Gopher would do, which should play well in our team. It's not a previously established standard you would have known.

Likewise I have suggested renaming the dashboard resource in a form that uses this app name and the correspondence in directory structure and resource names will be automatically checked soon (the script is in scripts/).


Other Notes

  • I really like that you called this one and have done a nice job with it. This app complements our product value especially well and I have enjoyed learning more about it from reviewing these PRs.
  • You have continued aggressively adopting dashboard features in your templates as they're rolled out... Fantastic!
  • I'd like to see us actually use the chart description field for useful descriptions, but ATM not looking to hold up this PR on that point and very cautious generally about requiring any more overhead in the process for current objectives.
  • One more point I loved here, you caught on to a mistake I made in handling the compose config, and fixed it in stride to keep us moving forward in a good way.

Really appreciate the delivery.

@nslaughter nslaughter enabled auto-merge (squash) September 19, 2023 20:56
@nodehatch
Copy link
Contributor Author

LGTM

@nslaughter nslaughter disabled auto-merge September 19, 2023 21:09
@nslaughter nslaughter merged commit bef5a9c into main Sep 19, 2023
@nslaughter nslaughter deleted the feat/collector/opa/dashboard branch September 19, 2023 21:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants