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

refactor(rosetta-icp): refactor and augment Rosetta ICP metrics #3642

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

Conversation

fsodre
Copy link
Contributor

@fsodre fsodre commented Jan 27, 2025

This PR:

  • Centralizes metrics set up and logic into a separate rosetta_core::metrics module, which can later be used to emit metrics from ICRC-Rosetta.
  • Injects a actix-web-prom wrapper into the REST server, which automatically creates http requests metrics and the /metrics endpoint.
  • Introduces a few new custom metrics: ledger_sync_blocks_fetched_total and ledger_sync_blocks_fetch_retries_total, besides the ones automatically added by actix-web-prom

@fsodre fsodre requested review from a team as code owners January 27, 2025 18:52
Copy link
Member

@mbjorkqvist mbjorkqvist left a comment

Choose a reason for hiding this comment

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

Thanks for this observability work @fsodre! What's the easiest way to test this? Is it simply spinning up the local cluster and seeing that things still work and perhaps seeing some data in some more Grafana dashboard panels, or should I be looking at the /metrics endpoint of the Rosetta instance, or something else?

There seem to be some CI failures that are not only related to formatting, so I'll make another pass once CI is green, but so far LGTM!

@@ -68755,6 +68830,44 @@
],
"license_file": "LICENSE-APACHE"
},
"strfmt 0.2.4": {
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit unfortunate that this unmaintained dependency has to be pulled in. It seems to only be used on this line in the actix-web-prom library. I had a quick look and found another couple of libraries similar to actix-web-prom, but they seem to be worse choices than actix-web-prom in terms on active maintenance, so I don't have any suggestions for improvement here.

@github-actions github-actions bot added the @idx label Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants