search

open-telemetry / opentelemetry-dotnet

The OpenTelemetry .NET Client
https://opentelemetry.io
Apache License 2.0
3.23k stars 765 forks source link

Unused ActivityListener #5809

Closed jamescrosswell closed 2 months ago

jamescrosswell commented 2 months ago

Not really a bug - just an unnecessary allocation: https://github.com/open-telemetry/opentelemetry-dotnet/blob/ba8a0e4c131054217555aae5c8a210da3a4b7c39/src/OpenTelemetry/Trace/TracerProviderSdk.cs#L26

It's a private field and the only time it ever gets used is when it gets disposed in either the Dispose or OnShutdown methods.

I think at some point it must have become hidden by this: https://github.com/open-telemetry/opentelemetry-dotnet/blob/39d960e0e77aa81c022bda0b33e1d23cf93a9f2d/src/OpenTelemetry/Trace/TracerProviderSdk.cs#L114

Kielek commented 2 months ago

I do not see any bug there. This listener is registered to ActivitySource then assigned to the field by https://github.com/open-telemetry/opentelemetry-dotnet/blob/39d960e0e77aa81c022bda0b33e1d23cf93a9f2d/src/OpenTelemetry/Trace/TracerProviderSdk.cs#L290-L291

Its logic is executed by ActivitySource.

jamescrosswell commented 2 months ago

I do not see any bug there. This listener is registered to ActivitySource then assigned to the field by https://github.com/open-telemetry/opentelemetry-dotnet/blob/39d960e0e77aa81c022bda0b33e1d23cf93a9f2d/src/OpenTelemetry/Trace/TracerProviderSdk.cs#L290-L291

Note quite. The listener that gets instantiated on line 114 (which is a local variable) is the one that gets added to the ActivitySource. The listener that is declared as a private field on line 26 gets instantiated in the constructor and is never used again (until it gets disposed).

Also worth noting that the listener that is instantiated on line 114 is never disposed of explicitly.

Kielek commented 2 months ago
  1. Line 26 - the field is uninitialized. There is no new instance of the ActivityListener.
  2. Line 114 - the new instance is created. It is configured in lines 116-288.
  3. Line 290 - instance created in line 114 is registered in ActivitySource
  4. Line 291 - instance created in line 114 is assigned to the field defined in line 26. 5.Line 367, 396 - field 26 is disposed. It was assigned in line 291.

Again, I do not see anything wrong. To mitigate confusions, please check #5810

jamescrosswell commented 2 months ago

4. Line 291 - instance created in line 114 is assigned to the field defined in line 26.

Ahh, I see. Understood. Sorry - my bad!