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

Update README to reflect more v12 changes #337

Closed
wants to merge 1 commit into from

Conversation

zbjornson
Copy link
Collaborator

Most significantly, adds a section about registerCollector.

Also some minor cleanup and condensing.

@sam-github
Copy link
Contributor

I wasn't thinking that registerCollector() would be used outside of the defaults, its a somewhat unfortunate interface (a version that supports callbacks would be better, but since .metrics() doesn't take a callback, that's not an option ATM), but it is what it is, and if prom-client wants to expose it I guess its at least simple and should be easy to keep stable.

@zbjornson
Copy link
Collaborator Author

since .metrics() doesn't take a callback, that's not an option ATM

The functions passed in to registerCollector have to be synchronous right now though, right? So we could easily do something like this:

const gauge = new client.Gauge({
  name: 'metric_name',
  help: 'metric_help',
  collect() {
    this.set(/* value or labels+value */);
  }
});

We could also support collect returning a Promise, in which case metrics would also have to return a Promise. clusterMetrics() is already async, so on the plus side, those interfaces would converge.

@zbjornson zbjornson mentioned this pull request Feb 27, 2020
@siimon
Copy link
Owner

siimon commented Feb 28, 2020

Thanks for doing this! Looks great and nice to get some input for someone that speaks english natively!

I don't see any reason for not exposing the registerCollector, guess it could be useful even with the sync limitation?

@sam-github
Copy link
Contributor

Yes, I guess maybe its useful. I'm not sure if people are adding it to the .ts and the README because they actually are using it (or want to), or just because its there.

The functions passed in to registerCollector have to be synchronous right now though, right?

Yes, but that's not because I thought it was easier, or didn't have the time to make them async, or anything like that.

They are sync because they are called by .metrics(), and it is sync (so everything it calls has to be sync, too).

It looked to me that making .metrics async was a pretty major change that would be guaranteed to break every single user of prom-client (at least, I can't think of a reason to use prom-client and not call .metrics(), maybe I'm missing something), unlike the other changes, which hit marginal (I hope) features, or features that had been deprecated for a fair while.

@Shudrum
Copy link

Shudrum commented Mar 4, 2020

Hey cool this update would have saved me some time to understand things ^^

BTW, the examples should be updated too. Do you mind to update it also, or I open a PR also?

Thank you!

@zbjornson
Copy link
Collaborator Author

Hey @Shudrum , thanks for pointing those out. I'm going to open a PR today with the async changes I'm proposing above, so before changing the examples, let's see if that change moves forward. :)

@Shudrum
Copy link

Shudrum commented Mar 5, 2020

Perfect! Thank you @zbjornson!

@mattolson
Copy link

I especially like that the inaccuracy around GC stats is updated. Let's get this merged!

@zbjornson
Copy link
Collaborator Author

Moot with #361

@zbjornson zbjornson closed this May 17, 2020
@zbjornson zbjornson deleted the zb/readme-12 branch May 17, 2020 00:47
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.

5 participants