Skip to content

svm repo split: fork metrics#7442

Merged
buffalojoec merged 1 commit intoanza-xyz:masterfrom
buffalojoec:svm-repo-split-fork-metrics
Aug 13, 2025
Merged

svm repo split: fork metrics#7442
buffalojoec merged 1 commit intoanza-xyz:masterfrom
buffalojoec:svm-repo-split-fork-metrics

Conversation

@buffalojoec
Copy link
Copy Markdown

The solana-metrics crate is a utility lib that's used across the validator to submit datapoints and other telemetry. Like solana-measure, it's not specific to any one component or domain of Agave, so it doesn't really make sense to live in SVM and have Agave import it.

One option, as this PR proposes, is to fork it just like we did with solana-measure in #7441. This is a pretty simple and reasonable route to take.

Another, more involved route is to just reimplement the datapoint module in the program-runtime locally, since it's the only SVM crate that uses solana-metrics (and just in one spot). However, this has two potential issues:

  • We end up reimplementing most of the lib anyway
  • If we need to use telemetry anywhere else in SVM, we would need to break it out of program-runtime anyway

Part of #7317

@mergify
Copy link
Copy Markdown

mergify Bot commented Aug 10, 2025

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@buffalojoec buffalojoec force-pushed the svm-repo-split-fork-metrics branch from 26de89c to 1e22da1 Compare August 11, 2025 17:19
@buffalojoec buffalojoec marked this pull request as ready for review August 11, 2025 17:19
@buffalojoec buffalojoec requested a review from a team as a code owner August 11, 2025 17:19
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.11792% with 211 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.2%. Comparing base (f6e6ff2) to head (1e22da1).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #7442     +/-   ##
=========================================
- Coverage    83.2%    83.2%   -0.1%     
=========================================
  Files         800      804      +4     
  Lines      362783   363563    +780     
=========================================
+ Hits       302195   302774    +579     
- Misses      60588    60789    +201     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@LucasSte
Copy link
Copy Markdown

I see most files are new. Did you simply copy solana-metrics into svm-metrics? Shouldn't we replace all usages in the validator by svm-metrics?

@buffalojoec
Copy link
Copy Markdown
Author

I see most files are new. Did you simply copy solana-metrics into svm-metrics? Shouldn't we replace all usages in the validator by svm-metrics?

Yes, it's a fork of the crate. I detailed in the description why I thought it made more sense to fork it. Let me know if it's reasonable.

@LucasSte
Copy link
Copy Markdown

I see most files are new. Did you simply copy solana-metrics into svm-metrics? Shouldn't we replace all usages in the validator by svm-metrics?

Yes, it's a fork of the crate. I detailed in the description why I thought it made more sense to fork it. Let me know if it's reasonable.

Oh, I got it. Is the goal to maintain two separate implementations then? One for Agave and another one for SVM?

@buffalojoec
Copy link
Copy Markdown
Author

Oh, I got it. Is the goal to maintain two separate implementations then? One for Agave and another one for SVM?

Yep, and we can thin each one out or evolve it based on requirements of each stack.

@buffalojoec buffalojoec merged commit c278df4 into anza-xyz:master Aug 13, 2025
51 checks passed
@buffalojoec buffalojoec deleted the svm-repo-split-fork-metrics branch August 13, 2025 13:34
@steviez
Copy link
Copy Markdown

steviez commented Aug 23, 2025

Like solana-measure, it's not specific to any one component or domain of Agave, so it doesn't really make sense to live in SVM and have Agave import it.

Remind me, why can't SVM just depend on the existing solana-metrics crate and why did we have to fork it ? solana-metrics doesn't depend on any other crates in Agave at the moment, so it is not like you're pulling in solana-core or agave-validator or anything:

agave/metrics/Cargo.toml

Lines 18 to 26 in bc650cf

[dependencies]
crossbeam-channel = { workspace = true }
gethostname = { workspace = true }
log = { workspace = true }
reqwest = { workspace = true, features = ["blocking", "brotli", "deflate", "gzip", "rustls-tls", "json"] }
solana-cluster-type = { workspace = true }
solana-sha256-hasher = { workspace = true }
solana-time-utils = { workspace = true }
thiserror = { workspace = true }

@buffalojoec
Copy link
Copy Markdown
Author

Remind me, why can't SVM just depend on the existing solana-metrics crate and why did we have to fork it ? solana-metrics doesn't depend on any other crates in Agave at the moment, so it is not like you're pulling in solana-core or agave-validator or anything:

I didn't want to end up in a situation like we have with spl-token, where SVM depends on some Agave libs and then Agave depends on SVM libs. This makes upgrades kind of annoying to manage.

We solved this for SPL with the SDK split-out. We could do the same here for metrics/telemetry, but I wasn't sure it was worth taking on that effort yet. As long as we're emitting the payloads, Grafana should pick it up, right?

Comment thread svm-metrics/src/metrics.rs
steviez pushed a commit that referenced this pull request Aug 31, 2025
mergify Bot pushed a commit that referenced this pull request Sep 2, 2025
This reverts commit c278df4.

(cherry picked from commit d7fdc8b)

# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	programs/sbf/Cargo.lock
steviez pushed a commit that referenced this pull request Sep 3, 2025
This reverts commit c278df4.

(cherry picked from commit d7fdc8b)

# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	programs/sbf/Cargo.lock
steviez added a commit that referenced this pull request Sep 4, 2025
… (#7824)

This reverts commit c278df4.

(cherry picked from commit d7fdc8b)

# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	programs/sbf/Cargo.lock

---------

Co-authored-by: Joe C <joe.caulfield@anza.xyz>
Co-authored-by: steviez <steven@anza.xyz>
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.

5 participants