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

Can't Add Collectors During Setup #11

Closed
Spinkelben opened this issue Oct 13, 2020 · 3 comments · Fixed by #12
Closed

Can't Add Collectors During Setup #11

Spinkelben opened this issue Oct 13, 2020 · 3 comments · Fixed by #12

Comments

@Spinkelben
Copy link

Spinkelben commented Oct 13, 2020

With the change to use DI for injecting the ICollectorRegistry the CollectorRegistryInstance property of PrometheusOptions becomes useless. We used to configure our metrics like this.

app.UsePrometheusServer(prometheusOptions =>
{
    prometheusOptions.CollectorRegistryInstance.Add(new MySystemMetricCollector());
});

This no longer works. The property is null. If I create a new CollectorRegistry in this method it is ignored. So I would suggest you set it before invoking the setup function.

@Spinkelben
Copy link
Author

Spinkelben commented Oct 13, 2020

Hmm, maybe I was too fast. I didn't fully understand the meaning of the ??= operator of this code:

options.CollectorRegistryInstance
                    ??= (ICollectorRegistry)app.ApplicationServices.GetService(typeof(ICollectorRegistry))
                    ?? Metrics.DefaultCollectorRegistry;

@Spinkelben
Copy link
Author

Spinkelben commented Oct 13, 2020

Ah, I found the real issue. I guess I was partially correct. We use DI to inject a IMetricFactory. That includes a ICollectorRegistry instance. Unless we use the same ICollectorRegistry instance in the setup method, it has no effect. This code would fix it:

app.UsePrometheusServer(prometheusOptions =>
{
    prometheusOptions.CollectorRegistryInstance = app.ApplicationServices.GetService<ICollectorRegistry>();
    prometheusOptions.CollectorRegistryInstance.Add(new MySystemMetricCollector());
});

However it is a bit stupid. It would be better, if we got the ICollectorRegistry instance from the PrometheusOptions object, when it is already registered in the DI container.

@sanych-sun
Copy link
Member

sanych-sun commented Oct 20, 2020

Hi @Spinkelben!

Sorry for the late response. If you are using DI integration library - the answer is simple: just remove the CollectorRegistryInstance property from options and it will be resolved from DI.

Currently we are using the following order:

  1. use collector registry from options
  2. try to resolve it from DI
  3. use static instance (as a fallback for backward compatibility).

So returning to the original issue: if you trying to register additional collectors - it would be better to resolve the ICollectorRegistry from DI somewhere on your side and add the collector to it. Or you can create instance of CollectorRegistry on your side, add your collector, and then use overloaded method to register MetricFactory by providing registry instance.

However you probably right - it will be better try to resolve the actual collector registry before calling configuration method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants