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

feat: allow to disable metric sort #108

Merged
merged 3 commits into from
Apr 14, 2023
Merged

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jan 29, 2023

This function does 50 % of a loading time (using inmemory adapter) so I'd like to be able to disable it.

@simPod simPod force-pushed the disbale-sort branch 2 times, most recently from c7746ab to 09b503c Compare January 29, 2023 13:58
Signed-off-by: Simon Podlipsky <simon@podlipsky.net>
@simPod
Copy link
Contributor Author

simPod commented Jan 31, 2023

disabling sort reduced the scrape duration by half

image

@LKaemmerling
Copy link
Member

LKaemmerling commented Jan 31, 2023

Hey @simPod,

good catch! I'm thinking of don't sort it per default. What do you think? Of course, this would be a bit different from what the other libs do (go/python) however it seems the sorting there is more efficient.

@simPod
Copy link
Contributor Author

simPod commented Jan 31, 2023

Yup but that would be BC. We can introduce this flag and flip the default value in next major? Your call.

@LKaemmerling LKaemmerling merged commit dce4295 into PromPHP:main Apr 14, 2023
@LKaemmerling
Copy link
Member

Thank you!

@simPod simPod deleted the disbale-sort branch April 14, 2023 13:00
@javespi
Copy link
Contributor

javespi commented Apr 18, 2023

Hello!

Yup but that would be BC. We can introduce this flag and flip the default value in next major? Your call.

I think it's still BC for any class which implements RegistryInterface. Use cases like mocks or wrappers.

After the latest minor release I am having this error:

PHP Fatal error:  Declaration of Javespi\MyCollectorRegistry::getMetricFamilySamples(): array must be compatible with Prometheus\RegistryInterface::getMetricFamilySamples(bool $sortMetrics = true): array

I think the only way of being BC is removing the type hint of the argument. I'll try to create a pull-request with a hotfix for this.


Edit: Reported as issue here #117
Edit 2: Pull request to avoid BC here #118

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.

3 participants