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

Export Metrics in WebRTCTransport #1878

Closed
justin0mcateer opened this issue Jul 12, 2023 · 4 comments
Closed

Export Metrics in WebRTCTransport #1878

justin0mcateer opened this issue Jul 12, 2023 · 4 comments
Labels
need/triage Needs initial labeling and prioritization

Comments

@justin0mcateer
Copy link

justin0mcateer commented Jul 12, 2023

  • Version:
    0.45.9

  • Platform:
    Linux master 6.3.5-arch1-1 #1 SMP PREEMPT_DYNAMIC Tue, 30 May 2023 13:44:01 +0000 x86_64 GNU/Linux
    Brave Browser 114.1.52.117

  • Subsystem:
    WebRTC Transport

Severity:

Low

Description:

When metrics exporting was implemented in the WebRTC module, it appears to have only be implemented in 'WebRTCDirectTransport' and not at all in 'WebRTCTransport'. Is there some reason for this or is it merely an oversight?

libp2p/js-libp2p-webrtc#64

Is there any reason we couldn't just 'port' the implementation from WebRTCDirectTransport to WebRTCTransport?

Steps to reproduce the error:

  • create an instance of libp2p with WebRTC() for the Transport
  • observe that no metrics are produced
@justin0mcateer justin0mcateer added the need/triage Needs initial labeling and prioritization label Jul 12, 2023
@maschad
Copy link
Member

maschad commented Jul 17, 2023

This is because our metrics library currently does not support browser metrics. Other users have collected browser metrics by forking prom-js so you could explore that option.

@maschad maschad closed this as completed Jul 17, 2023
@justin0mcateer
Copy link
Author

Yes, we have a Metrics implementation which works in the browser.

Since the Metrics implementation is injectable/injected, do you see any reason we can't simply port the existing code from WebRTCDirectTransport to WebRTCTransport?

@maschad
Copy link
Member

maschad commented Jul 17, 2023

You are correct, if your metrics implementation extends the metrics interface library then it should be possible. Would you like to open a PR?

@justin0mcateer
Copy link
Author

Indeed, we will.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

2 participants