-
-
Notifications
You must be signed in to change notification settings - Fork 95
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
chore(deps): update prometheus.client #2136
Conversation
Thanks for jumping in! Would you mind fixing the CI please? |
I ran it in my fork and all tests passed: So, something weird, I can't re-run it here... |
Step: Run Integration Tests
|
Re-triggered, let's see as it's odd indeed. |
Yes, I've just noticed so that must mean something is wrong with publishing the metrics. Quickly did #2144, let's see if it works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@tomkerkhove What about rewrite Scheduler to register jobs in runtime? I Can find time for this. It is not only for prometheus client https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection-guidelines#recommendations |
Good callout - I forgot to open an issue for that. Would you mind doing that with some context please? |
@tomkerkhove Sure, I will do. |
Fixes #1772
@tomkerkhove I can't remove
services.BuildServiceProvider();
without completely rewrite schedule logic. Right now I just updatedPrometheus.Client
and useDefaultCollectorRegistry
.Next step is rewrite scheduler or we can save it as is... For rewrite we should register Scheduler Job in runtime,
Configure
section instead ofConfigureServices
section, but I never usedCronScheduler.AspNetCore
. I found this kdcllc/CronScheduler.AspNetCore#29, maybe you know how to register in runtime better.