Skip to content

add txsSyncDurationTotal counter#6511

Merged
karknu merged 1 commit intomasterfrom
karknu/tx_sync_counter_main
Apr 8, 2026
Merged

add txsSyncDurationTotal counter#6511
karknu merged 1 commit intomasterfrom
karknu/tx_sync_counter_main

Conversation

@karknu
Copy link
Copy Markdown
Contributor

@karknu karknu commented Apr 7, 2026

Description

Add a txsSyncDurationTotal counter for tracking the total time spent syncing the mempool.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. These may include:
    • golden tests
    • property tests
    • roundtrip tests
    • integration tests
      See Running tests for more details
  • Any changes are noted in the CHANGELOG.md for affected package
  • The version bounds in .cabal files are updated
  • CI passes. See note on CI. The following CI checks are required:
    • Code is linted with hlint. See .github/workflows/check-hlint.yml to get the hlint version
    • Code is formatted with stylish-haskell. See .github/workflows/stylish-haskell.yml to get the stylish-haskell version
    • Code builds on Linux, MacOS and Windows for ghc-9.6 and ghc-9.12
  • Self-reviewed the diff

Note on CI

If your PR is from a fork, the necessary CI jobs won't trigger automatically for security reasons.
You will need to get someone with write privileges. Please contact IOG node developers to do this
for you.

@karknu karknu requested review from a team as code owners April 7, 2026 12:10
@karknu karknu force-pushed the karknu/tx_sync_counter_main branch from 7aec1b4 to 8044093 Compare April 7, 2026 12:12
Copy link
Copy Markdown
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

LGTM

My only experience with the counters was from implementing this commit January 9afc1e7

So I primarily just compared to that. The only difference I'm seeing is that this counter would not be maintained when the Mempool tracer is turned off. I think that's probably fine. (But happened to not be something we wanted for the January commit.)

I hesitate to Approve, only because I don't own this repository. But if someone who owns it feels comfortable to Approve based on this Comment, feel free.

[ IntM "txsSyncDuration" (round $ 1000 * duration)
let durationMs = round (1000 * duration) :: Integer in
[ IntM "txsSyncDuration" durationMs
, CounterM txsSyncDurationTotalCounterName (Just (fromIntegral durationMs))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For reference, I looked this up (for the nth time) just to check: Nothing is increment and Just n is +n

https://github.com/IntersectMBO/hermod-tracing/blob/0af5e41f81a6463a53b9ea109d85436b014de40e/trace-dispatcher/src/Cardano/Logging/Tracer/EKG.hs#L83-L84

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

EKG counters are integral. I think it's fine to round to milliseconds before contributing to the running total.

atomicModifyIORef' (fsTxsProcessedNum fs) $
\txCount -> join (,) $ f txCount

mapForgingStatsTxsSyncDuration ::
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Matches mapForgingStatsTxsProcessed just above ✅

@karknu
Copy link
Copy Markdown
Contributor Author

karknu commented Apr 8, 2026

So I primarily just compared to that. The only difference I'm seeing is that this counter would not be maintained when the Mempool tracer is turned off. I think that's probably fine.

I implemented it like the txsProcessedNum counter. It and the other mempool counters/gauges have the same behavior where they depend on the tracer being enabled.

Copy link
Copy Markdown
Contributor

@jutaro jutaro left a comment

Choose a reason for hiding this comment

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

No findings.

The new txsSyncDurationTotal metric looks consistent with the existing tracing/metrics pattern, and the naming/help-text updates match the behavior. Residual risk: I did not see tests for the new counter, so runtime/exporter behavior should still be checked manually.

@karknu karknu force-pushed the karknu/tx_sync_counter_main branch from 8044093 to b087110 Compare April 8, 2026 08:06
Add a txsSyncDurationTotal counter for tracking the total time spent
syncing the mempool.
@karknu karknu force-pushed the karknu/tx_sync_counter_main branch from b087110 to dd0eaf7 Compare April 8, 2026 08:08
@karknu karknu enabled auto-merge April 8, 2026 08:16
@karknu karknu added this pull request to the merge queue Apr 8, 2026
Merged via the queue into master with commit a9bf266 Apr 8, 2026
29 checks passed
@karknu karknu deleted the karknu/tx_sync_counter_main branch April 8, 2026 09:11
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.

3 participants