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

feat: add metrics property to helia interface #512

Merged
merged 1 commit into from
Apr 22, 2024
Merged

Conversation

achingbrain
Copy link
Member

To allow collecting metrics about arbitrary parts of the Helia stack, add an optional .metrics property for stat collection.

This can be used with implementations such as @libp2p/prometheus-metrics and/or others.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation if necessary (this includes comments as well)
  • I have added tests that prove my fix is effective or that my feature works

To allow collecting metrics about arbitrary parts of the Helia stack,
add an optional `.metrics` property for stat collection.

This can be used with implementations such as `@libp2p/prometheus-metrics`
and/or others.
@achingbrain achingbrain requested a review from a team as a code owner April 22, 2024 13:47
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

approved, but we should update readme with explainers on metrics collected, or a list of them at a minimum

Comment on lines +78 to +79
* A metrics object that can be used to collected arbitrary stats about node
* usage.
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we use libp2p metrics config object directly. Should we link to https://github.com/libp2p/js-libp2p/blob/main/doc/CONFIGURATION.md#configuring-metrics ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those docs need some updating, they're not correct any more.


/**
* A metrics object that can be used to collected arbitrary stats about node
* usage.
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +105 to +106
blocksSent: components.metrics?.registerCounter('helia_bitswap_sent_blocks_total'),
dataSent: components.metrics?.registerCounter('helia_bitswap_sent_data_bytes_total')
Copy link
Member

Choose a reason for hiding this comment

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

we should update readme with a list of metrics that we add when metrics are enabled.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be good, though prone to being out of date very quickly. It'd be nice if we could automate this somehow.

Comment on lines +15 to +18
this.blocksReceived = components.metrics?.registerMetricGroup('helia_bitswap_received_blocks')
this.duplicateBlocksReceived = components.metrics?.registerMetricGroup('helia_bitswap_duplicate_received_blocks')
this.dataReceived = components.metrics?.registerMetricGroup('helia_bitswap_data_received_bytes')
this.duplicateDataReceived = components.metrics?.registerMetricGroup('helia_bitswap_duplicate_data_received_bytes')
Copy link
Member

Choose a reason for hiding this comment

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

we should update readme with a list of metrics that we add when metrics are enabled.

@achingbrain achingbrain merged commit f7f71bb into main Apr 22, 2024
18 checks passed
@achingbrain achingbrain deleted the feat/add-metrics branch April 22, 2024 16:11
@achingbrain achingbrain mentioned this pull request Apr 22, 2024
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.

None yet

2 participants