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

Allow using a custom registry #26

Merged
merged 5 commits into from
Aug 8, 2022

Conversation

girstenbrei
Copy link
Contributor

Greets!
first of all, awesome work! This lib is my favorite way (across multiple languages, but especially in Rust) to write exporters. Just does exactly what i want.

One nice to have, would be #14 . My use case is to configure an application prefix, which is currently only possible by 'manually' adding it to every metric.
So, I started working on this and wanted to get some feedback. This implementation should work without braking any API. But, the currently global request metrics from the internal_metrics feature won't show up, as those are registered with the default registry, of course.

One solution is, to move those, maybe combined with the registry, as private fields inside the server struct. We could then reference it in handler_metrics and process_request, initializing the internal metrics in the Server::start.
Another one would be to use a new struct, just for passing this "bunch of metrics related things" around.

What do you think?

@AlexanderThaller
Copy link
Owner

AlexanderThaller commented Jun 30, 2022

Hello. Sorry for the delay. The pull-request looks good to me. Thanks for contributing.

@AlexanderThaller AlexanderThaller marked this pull request as ready for review June 30, 2022 12:22
@AlexanderThaller AlexanderThaller added the enhancement New feature or request label Jun 30, 2022
@AlexanderThaller AlexanderThaller modified the milestones: v0.9.0, v0.8.3, v0.8.5 Jun 30, 2022
@AlexanderThaller AlexanderThaller self-requested a review June 30, 2022 12:24
@AlexanderThaller
Copy link
Owner

AlexanderThaller commented Jun 30, 2022

I think one cargo +nightly fmt --all would be needed for the new example.

@AlexanderThaller AlexanderThaller modified the milestones: v0.8.5, v0.9.0 Jul 4, 2022
@girstenbrei
Copy link
Contributor Author

Hey there, no problem. So I added the docs line and ran formatting.

Regarding the internal_metrics feature: I added some documentation under the features section and the custom registry setter. This should make it clear, that this is not yet working.

I would really like to make the two work together, but this might be a bigger change, just because the metrics need to be passed through to where they are needed. Maybe better to do this in a separate change.

Thanks for your work!
Christoph

@AlexanderThaller AlexanderThaller self-requested a review July 11, 2022 08:31
@AlexanderThaller
Copy link
Owner

I think you can merge it now. Thank you for the contribution!

@AlexanderThaller AlexanderThaller modified the milestones: v0.9.0, v0.8.5 Jul 11, 2022
@girstenbrei
Copy link
Contributor Author

I think you can merge it now. Thank you for the contribution!

Sounds good! But I can't merge, no write access to this repo, so feel free to merge! Thx for creating the enhancement issue regarding the custom registry.

Very happy about the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants