Skip to content

PoC: Add metrics to TXM#503

Merged
GabrielMartinezRodriguez merged 13 commits intomasterfrom
gabriel/txm-telemetry
Apr 7, 2025
Merged

PoC: Add metrics to TXM#503
GabrielMartinezRodriguez merged 13 commits intomasterfrom
gabriel/txm-telemetry

Conversation

@GabrielMartinezRodriguez
Copy link
Contributor

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented Mar 7, 2025

Linked Issues

Added OpenTelemetry instrumentation to monitor txm

  • Implemented block monitoring metrics for current block number and block delay
  • Added nonce management metrics for tracking nonce allocation and queue status
  • Created transaction status change monitoring
  • Added transaction collector metrics
  • Implemented repository metrics for non-finalized transactions
  • Set up Prometheus exporter on port 9090 for metrics collection
Toggle Checklist

Checklist

Basics

  • B1. I have applied the proper label & proper branch name (e.g. norswap/build-system-caching).
  • B2. This PR is not so big that it should be split & addresses only one concern.
  • B3. The PR targets the lowest branch it can (ideally master).

Correctness

  • C1. Builds and passes tests.
  • C2. The code is properly parameterized & compatible with different environments.
  • C3. I have manually tested my changes & connected features.
  • C4. I have performed a thorough self-review of my code.

Architecture & Documentation

  • D1. I made it easy to reason locally about the code.
  • D2. All public-facing APIs & meaningful internal APIs are properly documented.
  • D3. Architecture is documented appropriately.
  • D4. Appropriate Changeset has been generated for npm published packages.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Mar 7, 2025

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1610388
Status:⚡️  Build in progress...

View logs

@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title Add OpenTelemetry metrics to TXM components PoC: Add metrics to TXM Mar 7, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez added the no-merge For showcase, not to be merged label Mar 7, 2025
description: "Count of transactions transitioning to a different status",
unit: "count",
valueType: ValueType.INT,
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this one useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because then we can track how many transactions we have in any state, as you can filter by the specific status. You can filter by status because when I add the counter, I specify the status:

transactionStatusChangeCounter.add(1, {
    status: this.status,
});

This actually generates a global counter for all statuses and one for each specific status

unit: "count",
valueType: ValueType.INT,
})

Copy link
Collaborator

@norswap norswap Mar 14, 2025

Choose a reason for hiding this comment

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

Seems to be missing more transaction metrics (processing, reverted, cancelled, successfull, and I would add failed in multiple buckets (RPC, timeout, simulation failure, ...), cf. notion doc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree with you. This was just to show what I think is a good way to implement metrics, but yes, we need to add more. I'm going to do it

description: "Number of transactions collected",
unit: "count",
valueType: ValueType.INT,
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Numba go up type of stuff. We can keep it, it's fun.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want a metric for the rate of growth over time? Like bucket into a time period (1 min? 10 min?) and see how many new txs arrive in that time and chart that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my intention, but to achieve that kind of metric, the right way to do it is to have a counter and then visualize the rate of increase of this counter over a period of time in the Grafana panel. This is the correct approach because it is more versatile and easier to implement

@norswap
Copy link
Collaborator

norswap commented Mar 14, 2025

This is good, probably add a few more metrics (esp. transaction breakdown) and then we can merge this initial batch.

@GabrielMartinezRodriguez GabrielMartinezRodriguez added reviewing-2 Ready for, or undergoing final review and removed no-merge For showcase, not to be merged labels Mar 17, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez marked this pull request as ready for review March 17, 2025 15:25
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/txm-telemetry branch 2 times, most recently from cf5bf43 to 96a7504 Compare March 18, 2025 14:17
This was referenced Mar 26, 2025
// biome-ignore format: tidy
console.warn(
"No signTransaction method found on the account, using signMessage instead. " +
"A viem update probably change the internal signing API.");
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting 🧐


return ResultAsync.fromThrowable(() => {
if (client.account.signTransaction) {
return client.account.signTransaction({
Copy link
Contributor

Choose a reason for hiding this comment

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

I think client.signTransaction should always be the correct approach. Some account types like JSON-RPC are essentially just a string as they defer signing elsewhere. client.signTransaction takes into account what type of account is configured and signs accordingly

(i think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was made a couple of months ago. But I remember that we tried to use client.account.signTransaction because using that method allowed us to avoid calling getChainId. I’m not exactly sure why, but when using client.signTransaction directly—even if you specify the chainId—it still makes a call to the blockchain

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's what I remember too. We should a comment there to explain why we're doing this though!

@norswap
Copy link
Collaborator

norswap commented Apr 4, 2025

Gave this a cursory look, good to merge from my side imho.
Just left a little comment to document our use of the internal signing function.

@norswap norswap added merge-after-changes Can be merged after requested changes are made and removed reviewing-2 Ready for, or undergoing final review labels Apr 4, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez merged commit b08d67b into master Apr 7, 2025
3 of 4 checks passed
@GabrielMartinezRodriguez GabrielMartinezRodriguez deleted the gabriel/txm-telemetry branch April 7, 2025 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-after-changes Can be merged after requested changes are made

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants