Skip to content

feat: add metrics#224

Closed
dapplion wants to merge 2 commits intomasterfrom
dapplion/metrics
Closed

feat: add metrics#224
dapplion wants to merge 2 commits intomasterfrom
dapplion/metrics

Conversation

@dapplion
Copy link
Copy Markdown
Contributor

@dapplion dapplion commented Oct 8, 2022

Minimal set of metrics to track errors and measure packet rate

  • xxHandshakeSuccesses: Total count of xxHandshake successes
  • xxHandshakeErrors: Total count of xxHandshake errors
  • encryptedPackets: Total count of successfully encrypted packets
  • decryptedPackets: Total count of successfully decrypted packets
  • decryptErrors: Total count of errors decrypting packets

@dapplion dapplion requested a review from a team as a code owner October 8, 2022 14:40
@dapplion dapplion changed the title Add metrics feat: add metrics Oct 8, 2022
Copy link
Copy Markdown
Collaborator

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

Can these metrics please get added to the built in libp2p metrics object instead of being their own thing?

@dapplion
Copy link
Copy Markdown
Contributor Author

dapplion commented Oct 14, 2022

ComponentMetricsUpdate is quite strange, how does this connects to Prometheus? Why is this data structure so nested? A single metric name would be sufficient, since those must never have collisions in Prometheus

The approach of ComponentMetricsUpdate looks more expensive than the simple metric.inc() this PR does. Given that this is done per package can be significant. What's the point of having a "central" place of metrics?

Note gossipsub rolls its own metrics system too https://github.com/ChainSafe/js-libp2p-gossipsub/blob/master/src/metrics.ts

@dapplion
Copy link
Copy Markdown
Contributor Author

Updated PR to use a "metrics generator" so that consumer only needs to pass something that can create Gauge instances. This follows similar encapsulation principles as built in libp2p metrics object but with no performance penalty.

@mpetrunic
Copy link
Copy Markdown
Member

superseeded by #246

@mpetrunic mpetrunic closed this Nov 15, 2022
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