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

fix: Use Grafana provisioning for metrics #1625

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Ktl-XV
Copy link

@Ktl-XV Ktl-XV commented Jan 8, 2025

What was wrong?

As reported in #1551 the create-dashboard trin sub command stopped working.

The issue arose because Grafana and nanotemplate both use brackets for template placeholders. A change in the dashboard introduced new metrics which included Grafana placeholders, but nanotemplate was parsing them as it's own.

How was it fixed?

Since Grafana includes a provisioning system, switched to using that and removed the create-dashboard trin sub command. This also removed the need for nanotemplate as a dependency.

Additionally, updated the dashboard's panels to display State network data (and any networks that might be added later)

To-Do

@KolbyML
Copy link
Member

KolbyML commented Jan 8, 2025

I am not the biggest fan of this approach because it makes the code/instructions platform specific, whereas before you could use the subcommand on Windows as well

@Ktl-XV Ktl-XV force-pushed the grafana-provisioning branch from fba2fab to f518cc2 Compare January 8, 2025 14:36
@Ktl-XV
Copy link
Author

Ktl-XV commented Jan 8, 2025

Thanks for the feedback.

That's right, didn't think about Windows. Since currently the monitoring instructions are Linux and Docker only, that is where I focused.

Could a solution be if/when adding Windows instructions, add a step to edit the provisioning file and point to the appropriate location?

A different option could be to include an importable dashboard where the user pastes the JSON in Grafana and the web UI guides them to select a Prometheus data source (which can still be provisioned automatically). This approach would work in any OS while also making it easier to add the Trin Dashboard to Grafana instances without having to input credentials into the terminal. This template could also be uploaded to the Grafana Dashboards website to make it easier to import.

Or we could also replace nanotemplate with another templating engine which can handle this use case properly and keep the subcommand. I don't like this approach much because, imho, configuring a Grafana instance should be out of scope of the main software, but I'm willing to do it if you prefer this option.

@KolbyML
Copy link
Member

KolbyML commented Jan 8, 2025

Thanks for the feedback.

That's right, didn't think about Windows. Since currently the monitoring instructions are Linux and Docker only, that is where I focused.

Could a solution be if/when adding Windows instructions, add a step to edit the provisioning file and point to the appropriate location?

A different option could be to include an importable dashboard where the user pastes the JSON in Grafana and the web UI guides them to select a Prometheus data source (which can still be provisioned automatically). This approach would work in any OS while also making it easier to add the Trin Dashboard to Grafana instances without having to input credentials into the terminal. This template could also be uploaded to the Grafana Dashboards website to make it easier to import.

Or we could also replace nanotemplate with another templating engine which can handle this use case properly and keep the subcommand. I don't like this approach much because, imho, configuring a Grafana instance should be out of scope of the main software, but I'm willing to do it if you prefer this option.

Assuming the subcommand worked (it used to), didn't it work on Windows?

@KolbyML
Copy link
Member

KolbyML commented Jan 8, 2025

If you want a think I good solution would be doing what I did for Trin Execution https://github.com/ethereum/trin/tree/master/trin-execution/metrics

You can rename the folder from etc to something like metrics or dashboards to be more readable.

This way anybody could run metrics easier then our current system for trin, just docker compose up and they would be live

@Ktl-XV
Copy link
Author

Ktl-XV commented Jan 8, 2025

I agree, using docker compose up is way simpler.
I'll update the PR.
Thanks

- The `--web-transport http` will allow Grafana to request routing table information from Trin via JSON-RPC over HTTP

4. Navigate to http://localhost:3000/d/trin-metrics/trin-metrics. Use `admin`/`admin` to login.


## Metrics setup no docker
Copy link
Member

Choose a reason for hiding this comment

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

If you want you can also delete the ## Metrics setup no docker, I don't think we should keep it as

  • it is more complex to maintain especially as people are unlikely to use it so nobody will know how it works so they can fix it
  • the docker solution is just better

No worries if you don't want to include that in this PR, we can always remove it on a different date

Copy link
Author

Choose a reason for hiding this comment

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

I'll go ahead and remove it, that way the structure of the new metrics dir can be the same as the one in trin-execution

@Ktl-XV Ktl-XV changed the title Use Grafana provisioning for metrics fix: Use Grafana provisioning for metrics Jan 9, 2025
@Ktl-XV Ktl-XV force-pushed the grafana-provisioning branch from 10240a9 to 9c18fd8 Compare January 9, 2025 19:34
@Ktl-XV
Copy link
Author

Ktl-XV commented Jan 9, 2025

Fixed the commit messages.

Thanks @KolbyML for fixing CI

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