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

Enable resource manager metrics by default #2372

Closed
Tracked by #2326
sukunrt opened this issue Jun 15, 2023 · 9 comments
Closed
Tracked by #2326

Enable resource manager metrics by default #2372

sukunrt opened this issue Jun 15, 2023 · 9 comments
Labels
effort/hours Estimated to take one or several hours exp/beginner Can be confidently tackled by newcomers kind/enhancement A net-new feature or improvement to an existing feature

Comments

@sukunrt
Copy link
Member

sukunrt commented Jun 15, 2023

We have found these metrics very useful and the cost of reporting metrics is not a lot. Users can opt out if they want to. In line with the rest of the places in libp2p we should enable these metrics by default too.

@sukunrt sukunrt added the kind/discussion Topical discussion; usually not changes to codebase label Jun 15, 2023
@marten-seemann
Copy link
Contributor

Sounds good to me, let's do it!

@marten-seemann marten-seemann added kind/enhancement A net-new feature or improvement to an existing feature exp/beginner Can be confidently tackled by newcomers effort/hours Estimated to take one or several hours and removed kind/discussion Topical discussion; usually not changes to codebase labels Jun 19, 2023
@marten-seemann marten-seemann mentioned this issue Jun 19, 2023
29 tasks
@sukunrt
Copy link
Member Author

sukunrt commented Jun 22, 2023

The current implementation of TraceReporter is in a separate package github.com/libp2p/go-libp2p/p2p/host/resource-manager/obs which depends on rcmgr package.

So from within rcmgr there is no way to initialise a new StatsTraceReporter.

There are two options here:

  1. Move obs.StatsTraceReporter to the same package. This would be a breaking change
  2. Add a methods like Init on rcmgr which is called while setting up BasicHost

I lean towards 1 since users using StatsTraceReporter don't need to pass it explicity after this any way.

@MarcoPolo
Copy link
Collaborator

Can you do 1 without a breaking change? I.e both ways work

@sukunrt
Copy link
Member Author

sukunrt commented Jun 23, 2023

@MarcoPolo I think I can. Can you check #2388 and #2389 ?

I've moved the type StatsTraceReporter to rcmgr package and created an alias in package obs to the this rcmgr type like

package obs 

type StatsTraceReporter = rcmgr.StatsTraceReporter

kubo works fine with this. I think this should be fine.

@marten-seemann
Copy link
Contributor

Can you do 1 without a breaking change? I.e both ways work

Why? This is only used by Kubo and Lotus (and maybe a very small number of consumers). I'd rather do the right thing, instead of creating additional API surface for the sake of backwards compatibility.

@MarcoPolo
Copy link
Collaborator

Why? This is only used by Kubo and Lotus (and maybe a very small number of consumers). I'd rather do the right thing, instead of creating additional API surface for the sake of backwards compatibility.

I'd argue that the right thing is avoiding unnecessary breakages for users. They've already jumped through the hoops of setting this up once, let's not force a harder upgrade for them.

@marten-seemann
Copy link
Contributor

Building in backwards compatibility comes with a long-term maintenance cost for us. Since we're creating the PRs for Kubo and Lotus anyway, there's no cost for them to pay in the first place.

If we want to avoid breaking users, we should at least deprecate the option that we don't want people to use any more.

@marten-seemann
Copy link
Contributor

#2388 deprecated the obs package, so this should be fine.

@marten-seemann
Copy link
Contributor

Fixed by #2389.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/hours Estimated to take one or several hours exp/beginner Can be confidently tackled by newcomers kind/enhancement A net-new feature or improvement to an existing feature
Projects
None yet
Development

No branches or pull requests

3 participants