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

AddMetrics can initialize MetricsSubscriptionManager multiple times #90779

Closed
JamesNK opened this issue Aug 18, 2023 · 1 comment · Fixed by #90902
Closed

AddMetrics can initialize MetricsSubscriptionManager multiple times #90779

JamesNK opened this issue Aug 18, 2023 · 1 comment · Fixed by #90902

Comments

@JamesNK
Copy link
Member

JamesNK commented Aug 18, 2023

Description

Calling AddMetrics initializes the subscription manager:

// Make sure the subscription manager is started when the host starts.
// The host will trigger options validation.
services.AddOptions<NoOpOptions>().Configure<MetricsSubscriptionManager>((_, manager) => manager.Initialize()).ValidateOnStart();

However, the way this is done isn't resilient to multiple calls to AddMetrics by app or library code.

Listeners are initialized multiple times:

public void Initialize()
{
foreach (var listener in _listeners)
{
listener.Initialize();
}
}

Reproduction Steps

services.AddMetrics();
services.AddMetrics();

Expected behavior

Multiple IConfigurateOptions<MetricsServiceExtensions+NoOpOptions> aren't registered in DI. MetricsSubscriptionManager is initialized once.

Actual behavior

Multiple IConfigurateOptions<MetricsServiceExtensions+NoOpOptions> is registered in DI. Each will instance will call initialize on MetricsSubscriptionManager.

I noticed this when the AddMvc_Twice_DoesNotAddDuplicates test in aspnetcore started throwing because there were duplicate config options.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 18, 2023
@Tratcher Tratcher added this to the 8.0.0 milestone Aug 21, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 21, 2023
@Tratcher
Copy link
Member

Tratcher commented Aug 21, 2023

This doesn't seem critical for rc1, there aren't currently any implementations of IMetricsListener.Initialize other than the console, and the early adopters can mitigate this in their own code until rc2.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 21, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 22, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants