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: Add support for metrics collection #1762

Merged
merged 23 commits into from
Apr 9, 2024

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Mar 12, 2024

This PR introduces a package for instrumenting the code for metrics collection. The new README details the environment variables that need to be set and includes an example of how one might configure their local pipeline to export metrics to Grafana Cloud.

I've added a few metrics in the tm2 package; I think they are in the correct spots.

You may notice that the metrics subfolder in the telemetry package is unnecessarily modular. This is because this package originally include code for gathering traces as well. There is an existing issue with this and have decide to presently exclude it from this PR until it is fixed and/or requested -- this is a feature we would almost certainly not want to enable in a production environment. Metrics collection is more likely to be enabled in prod because they can be set to be exported to a local collector application that handles more complex batching, retry requests, and publishing to the end data store, in this case Grafana.

Contributors' checklist... - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [x] Provided any useful hints for running manual tests - [x] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).

@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Mar 12, 2024
telemetry/metrics/init.go Outdated Show resolved Hide resolved
telemetry/init.go Show resolved Hide resolved
gno.land/cmd/gnoland/start.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/start.go Show resolved Hide resolved
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

I apologize for being late to the party, I realize this PR has been gathering dust on the shelf for a time now 🙁

Thank you for adding this functionality, it's something I personally see using very frequently. It's also a nice foundation for future (much more robust) metrics in the node 🙏

Pinging @ajnavarro / @gfanton to give a second review. I think this is good to go 🚀

gno.land/cmd/gnoland/start.go Outdated Show resolved Hide resolved
telemetry/metrics/init.go Outdated Show resolved Hide resolved
telemetry/metrics/init.go Outdated Show resolved Hide resolved
telemetry/metrics/init.go Outdated Show resolved Hide resolved
telemetry/metrics/init.go Outdated Show resolved Hide resolved
telemetry/metrics/init.go Outdated Show resolved Hide resolved
telemetry/options/config.go Outdated Show resolved Hide resolved
telemetry/options/config.go Show resolved Hide resolved
telemetry/init.go Show resolved Hide resolved
telemetry/README.md Outdated Show resolved Hide resolved
@zivkovicmilos
Copy link
Member

Thank you @mazzy89 for the review 🙏

Copy link
Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

LGTM after resolving all comments 👍

@deelawn
Copy link
Contributor Author

deelawn commented Mar 26, 2024

Finally got this tested and sending metrics to Grafana thanks to @mazzy89 🙇 🙏 🎉

@deelawn deelawn requested a review from gfanton as a code owner April 8, 2024 19:13
Copy link

codecov bot commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 47.74%. Comparing base (59c6d3e) to head (4e2edd6).

Files Patch % Lines
gno.land/cmd/gnoland/start.go 60.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1762   +/-   ##
=======================================
  Coverage   47.74%   47.74%           
=======================================
  Files         393      393           
  Lines       61637    61647   +10     
=======================================
+ Hits        29430    29436    +6     
- Misses      29736    29738    +2     
- Partials     2471     2473    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zivkovicmilos
Copy link
Member

@deelawn feel free to merge when you get 1 more approve by @gfanton / @ajnavarro 🙏

Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

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

Not much to add after the other reviews. LGTM 👍

@deelawn deelawn merged commit 7286ca7 into gnolang:master Apr 9, 2024
191 of 193 checks passed
@deelawn deelawn deleted the feat/oltp-metrics branch April 9, 2024 19:02
@moul moul mentioned this pull request Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants