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

Metrics: Add algod version to metrics #6003

Merged
merged 4 commits into from
Jun 26, 2024

Conversation

hsoerensen
Copy link
Contributor

@hsoerensen hsoerensen commented May 23, 2024

Summary

This PR adds algod version information to metrics exported. This will allow querying relay servers to see if they have updated to required / requested versions, and potentially be used to enforce SLA's around patching.

This PR does introduce a new namespace 'environment' for metrics, with reasoning being that this namespace is for exposing app-level runtime environment information, which currently is not exposed (outside of what Prometheus's node_exporter may provide at an OS level).

One label is used (version). I was thinking of splitting this up into labels with version (major.minor.patch), build, OS, arch, etc - however without having a clear use-case as to why this would be required I opted for simplicity. Happy to adjust as designated labels would be a more stable interface for anyone needing these data, if that is expected.

Also note that the string is VERY similar to a duplicated user-agent header strings in wsNetwork.go and p2p.go (e.g. https://github.com/algorand/go-algorand/blob/master/network/p2p/p2p.go#L85), however the existing format makes it more difficult to parse the string in an observability platform, if goal is to map back to release versions published (e.g., 3.24.0) vs parsing in the format contained in this PR. I opted not to change user-agent strings as that could be seen as a breaking change, in case anyone parses these.

Test Plan

  • I ran a local algod version, and confirmed that the data was in there.
  • Ran make test with no failures

@gmalouf gmalouf force-pushed the add-algod-version-to-metrics branch from 70e04b5 to 6921204 Compare June 11, 2024 18:18
@gmalouf gmalouf changed the title Add algod version to metrics Metrics: Add algod version to metrics Jun 11, 2024
@gmalouf gmalouf self-assigned this Jun 11, 2024
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 55.86%. Comparing base (24382d8) to head (c2c11c1).
Report is 1 commits behind head on master.

Files Patch % Lines
daemon/algod/server.go 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6003      +/-   ##
==========================================
- Coverage   55.87%   55.86%   -0.01%     
==========================================
  Files         482      482              
  Lines       68593    68602       +9     
==========================================
- Hits        38324    38323       -1     
- Misses      27658    27673      +15     
+ Partials     2611     2606       -5     

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

gmalouf
gmalouf previously approved these changes Jun 11, 2024
@gmalouf gmalouf requested a review from algorandskiy June 11, 2024 21:36
@gmalouf
Copy link
Contributor

gmalouf commented Jun 12, 2024

This will work as-is, but you may want to use a gauge rather than a counter semantically.

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

I think this is too much info for metric but I guess it could work. Maybe /status is a better fit for it? I defer to others to weigh it in.

daemon/algod/server.go Outdated Show resolved Hide resolved
config/version.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

Could you split up the metric into multiple labels and use a gauge?

daemon/algod/server.go Outdated Show resolved Hide resolved
@hsoerensen hsoerensen force-pushed the add-algod-version-to-metrics branch from 6921204 to f6b9842 Compare June 25, 2024 19:43
@hsoerensen hsoerensen requested a review from cce June 25, 2024 20:37
@cce cce closed this Jun 25, 2024
@cce cce reopened this Jun 25, 2024
daemon/algod/server.go Outdated Show resolved Hide resolved
cce
cce previously approved these changes Jun 26, 2024
daemon/algod/server.go Outdated Show resolved Hide resolved
daemon/algod/server.go Outdated Show resolved Hide resolved
@algorandskiy algorandskiy merged commit f523300 into algorand:master Jun 26, 2024
32 checks passed
@hsoerensen hsoerensen deleted the add-algod-version-to-metrics branch June 26, 2024 21:50
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.

4 participants