Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

feat: export metrics #71

Merged
merged 21 commits into from
May 9, 2023

Conversation

lightsofapollo
Copy link
Contributor

@lightsofapollo lightsofapollo commented Jan 4, 2023

Adds metrics for common error cases (timeoout, dtls handshake failure) and few success cases (stream open / stream close)

Closes: #64

@lightsofapollo lightsofapollo marked this pull request as ready for review January 4, 2023 18:10
@lightsofapollo
Copy link
Contributor Author

@achingbrain David DiMaria pointed out to me that we already have this : https://github.com/libp2p/js-libp2p-interfaces/blob/master/packages/interface-mocks/src/metrics.ts . I believe sinon-ts was suggested in another comment but using the existing mock would definitely work. Do you have a preference?

@achingbrain
Copy link
Member

achingbrain commented Jan 5, 2023

They are intended to fulfil different roles - the classes in @libp2p/interface-mocks are functioning implementations of the various interfaces (e.g. they pass their associated interface tests), useful for when stubbing large amounts of functionality would make the test overly verbose or would introduce lots of redundant code.

Previously we had complicated dependency trees where libp2p components would depend on each other to test functionality which made releasing changes across multiple modules difficult, the collection of mocks was intended to alleviate some of that.

You'd use sinon-ts when you want to assert simple interactions with the other components, like a method being invoked with certain arguments being passed, etc which seemed like a better fit here.

@ddimaria
Copy link
Collaborator

@achingbrain This PR collects dialer events. I know we'll need to add listener ones when b2b is completed. Can you think of other relevant events to include? (tcp had a server status that isn't applicable here).

@BigLep
Copy link

BigLep commented Jan 16, 2023

@achingbrain : I want to get this one out of limbo. Can you please give any input?

@achingbrain achingbrain changed the title Feat/64/export metrics feat: export metrics Feb 2, 2023
@p-shahi
Copy link
Member

p-shahi commented Feb 10, 2023

@achingbrain @lightsofapollo What are the next steps to get this landed?

@ddimaria
Copy link
Collaborator

@achingbrain @lightsofapollo What are the next steps to get this landed?

@lightsofapollo it looks like there are conflicts that need to be resolved, and an issue with CI. I have an outstanding question to @achingbrain, though maybe @MarcoPolo can answer it?

@p-shahi
Copy link
Member

p-shahi commented Feb 24, 2023

I'll bring this up for discussion during the js-libp2p triage to determine next steps

@ddimaria
Copy link
Collaborator

ddimaria commented Mar 7, 2023

I'll bring this up for discussion during the js-libp2p triage to determine next steps

@p-shahi were you able to determine the next steps?

@p-shahi
Copy link
Member

p-shahi commented Mar 7, 2023

I'll bring this up for discussion during the js-libp2p triage to determine next steps

@p-shahi were you able to determine the next steps?

Yes, @wemeetagain is assigned for review, so waiting on that.

@maschad maschad assigned maschad and unassigned wemeetagain Mar 14, 2023
@maschad maschad self-requested a review March 15, 2023 19:56
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

LGTM

@maschad maschad force-pushed the feat/64/export-metrics-reland branch from 28c9183 to 29a9269 Compare March 15, 2023 20:55
@maschad maschad self-requested a review March 15, 2023 21:05
maschad
maschad previously approved these changes Mar 16, 2023
@maschad
Copy link
Member

maschad commented Mar 20, 2023

It seems the failing test has been resolved on the Browser-to-server PR , regardless there will be merge conflicts so let's wait till that is merged first given that's of a higher priority.

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

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

Looks like these are the events that are tracked:
incoming_stream
stream_end
close
openError
unknownError

  • Consider using either camel case OR underscore case.
  • Consider tracking a new outgoing stream
  • Consider tracking opening a peer connection

@maschad maschad self-requested a review March 27, 2023 20:41
@maschad maschad dismissed their stale review March 29, 2023 01:14

Recommendation of additional metrics by @wemeetagain

@maschad maschad added the enhancement New feature or request label May 1, 2023
Copy link
Member

@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.

LGTM, just a few small nits

test/transport.browser.spec.ts Outdated Show resolved Hide resolved
src/transport.ts Outdated Show resolved Hide resolved
src/transport.ts Outdated Show resolved Hide resolved
@maschad
Copy link
Member

maschad commented May 4, 2023

#150 has the necessary fixes to stop CI from failing, let's merge that first.

@maschad maschad merged commit b3cb445 into libp2p:main May 9, 2023
github-actions bot pushed a commit that referenced this pull request May 9, 2023
## [1.2.0](v1.1.11...v1.2.0) (2023-05-09)

### Features

* export metrics ([#71](#71)) ([b3cb445](b3cb445))
@github-actions
Copy link

github-actions bot commented May 9, 2023

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request released
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Export metrics
7 participants