[api-logs] Remove InstrumentationScope class #4436
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4436 +/- ##
==========================================
+ Coverage 84.77% 84.95% +0.18%
==========================================
Files 307 306 -1
Lines 12158 12154 -4
==========================================
+ Hits 10307 10326 +19
+ Misses 1851 1828 -23
|
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the <see cref="OpenTelemetry.InstrumentationScope"/> associated | ||
| /// with the logger. | ||
| /// Gets the name identifying the instrumentation library. |
There was a problem hiding this comment.
XML comments can be clean up in a later PR, but just noting that...
We can still refer to this as instrumentation scope here. Something like
| /// Gets the name identifying the instrumentation library. | |
| /// Gets the instrumentation scope name of the Logger. |
or even just dropping the term instrumentation scope, which mimics the docs for Meter
| /// Gets the name identifying the instrumentation library. | |
| /// Gets the Logger name. |
There was a problem hiding this comment.
I don't disagree with you that they suck 🤣 but I did keep it ~consistent with what we have in TracerProvider:
opentelemetry-dotnet/src/OpenTelemetry.Api/Trace/TracerProvider.cs
Lines 41 to 42 in 2914002
I think I'll leave it alone for now and then in a follow-up we can fix it all up?
There was a problem hiding this comment.
I think I'll leave it alone for now and then in a follow-up we can fix it all up?
Totally, and you're right we'd probably benefit from an editorial pass over our XML docs in general.
| /// <param name="name">Optional name identifying the instrumentation library.</param> | ||
| /// <param name="logger"><see cref="Logger"/>.</param> | ||
| /// <returns><see langword="true"/> if the logger was created.</returns> | ||
| protected virtual bool TryCreateLogger( |
There was a problem hiding this comment.
This TryCreateLogger may look a little strange, but it accomplishes a number of things...
- There is only a single
virtualmethod that must be implemented in SDK(s). - We can add other
GetLoggermethods freely that add other things to the instrumentation scope (schema_url, attributes, etc.) maintained onLoggerwithout implementations breaking. - There is some stuff in the spec about repeat calls returning the same instances and mutating instances when config changes. We don't support that yet, but in theory separating "Get" from "Create" should allow the base class to implement that stuff if we ever need to.
| /// <returns><see langword="true"/> if the logger was created.</returns> | ||
| protected virtual bool TryCreateLogger( | ||
| string? name, | ||
| #if NETSTANDARD2_1_OR_GREATER || NET6_0_OR_GREATER |
There was a problem hiding this comment.
A note about this condition...
OpenTelemetry.Api currently targets netstandard2.0;net462 so this will never resolve to true for any binary in the package. @TimothyMothra is currently exploring how to best approach this (eg #4293). Once we have a more recent target (or two) this should light up. In the meantime nothing catastrophic will happen just some wonky false-positive warnings about possibly accessing a null variable.
Relates to #4433
Changes
class InstrumentationScope(some early feedback from @alanwest)LoggerProviderMerge requirement checklist