[Logs] Updates for specification changes #3677
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3677 +/- ##
==========================================
- Coverage 87.74% 87.41% -0.33%
==========================================
Files 283 295 +12
Lines 10286 10542 +256
==========================================
+ Hits 9025 9215 +190
- Misses 1261 1327 +66
|
| /// <summary> | ||
| /// LoggerProvider is the entry point of the OpenTelemetry API. It provides access to <see cref="Logger"/>. | ||
| /// </summary> | ||
| public class LoggerProvider : BaseProvider |
There was a problem hiding this comment.
We're very confident that LoggerProvider will be the final term? Seems very likely, but just calling out this is an area of risk if we release 1.4 stable and then it changes.
There was a problem hiding this comment.
I feel like yes, but can't say for certain! I know keeping it consistent with traces & metrics was one of the goals. If we want to make LoggerProvider and LoggerProviderBuilder internal what we would lose is the ability to release the Serilog & EventSource extensions to get feedback. For those to work you have to pass in the provider instance.
src/OpenTelemetry.Extensions.EventSource/OpenTelemetryEventSourceLogEmitter.cs
Outdated
Show resolved
Hide resolved
| internal Logger GetLogger(string? name) | ||
| => this.GetLogger(new LoggerOptions(name)); | ||
|
|
||
| internal Logger GetLogger(InstrumentationScope instrumentationScope) |
There was a problem hiding this comment.
Since metrics and traces do not yet support InstrumentationScope would it make sense to just have
internal Logger GetLogger(string name, string version)This mirrors .NET's support for meters and activities. When the runtime introduces support for schema url and attributes, we should follow the same pattern for the logger.
Also, I understand InstrumentationScope is internal right now, so not a big deal if this merges as is.
There was a problem hiding this comment.
This one was kind of an interesting journey ⛵
Check out: Get a Logger. There are a couple unique parameters event_domain (required if you want to emit events) & include_trace_context (optional, true by default). I think the first natural thing to do is like...
internal Logger GetLogger(string? name = null, string? version = null, string? eventDomain = null, bool includeTraceContext = true) {}But then what do we do if we want to support the other scope things (schema_url, attributes) in the future? We have to drop in another giant overload, or have a whole bunch of non-defaulted versions to try and cover all the permutations. If the spec ever adds anything else, more trouble.
So in order to keep it simple and allow for expansion I originally just had:
Logger GetLogger(LoggerOptions options) {}internal sealed class LoggerOptions
{
public LoggerOptions(string? name) {}
public string Name { get; }
public string? Version { get; init; }
public string? SchemaUrl { get; init; }
public IReadOnlyDictionary<string, object>? Attributes { get; init; }
public string? EventDomain { get; init; }
public bool IncludeTraceContext { get; init; } = true;
}But then, Name + Version + SchemaUrl + Attributes are really "instrumentation scope" which (by the spec) also apply to tracing & metrics. So to achieve potential re-use it became...
internal sealed class LoggerOptions
{
public LoggerOptions(InstrumentationScope instrumentationScope) {}
public InstrumentationScope InstrumentationScope { get; }
public string? EventDomain { get; init; }
public bool IncludeTraceContext { get; init; } = true;
}I was thinking at some point we might do...
class LoggerProvider
{
public Tracer GetTracer(string name, string version = null) {}
+ public Tracer GetTracer(InstrumentationScope instrumentationScope) {}
}Then I started to write tests and update the code to use this API. When you just want to pass a name having to new up LoggerOptions I felt was too ceremonious. So I add some helpers for the common cases.
internal Logger GetLogger()
=> this.GetLogger(name: null);
internal Logger GetLogger(string? name)
=> this.GetLogger(new LoggerOptions(name));
internal Logger GetLogger(InstrumentationScope instrumentationScope)
=> this.GetLogger(new LoggerOptions(instrumentationScope));Worth noting here is the InstrumentationScope does end up on LogRecord so exporters could actually be authored to be spec complaint w.r.t. "instrumentation scope".
So what should we do here? 🤷 It is all internal so we could just wait and see 😄
There was a problem hiding this comment.
It is all internal so we could just wait and see 😄
Yes. I think it would be ideal if these things were encapsulated in a .NET class not an OpenTelemetry class. Relevant to dotnet/runtime#63651 - the API here should be expanded to include the scope attributes.
| configure!(resourceBuilder); | ||
| } | ||
|
|
||
| public void SetResourceBuilder(ResourceBuilder resourceBuilder) |
There was a problem hiding this comment.
I personally like the ConfigureResource pattern over this. Does it make sense to just remove it from logging, or do you think the lack of parity with our trace/metric support is bad?
There was a problem hiding this comment.
I'm not totally convinced on the uselessness of SetResourceBuilder. If you look at any individual signal in a vacuum, it totally makes sense to use ConfigureResource. But if you are configuring a shared/global resource which will be used by two or more signals, or two or more providers of the same signal, isn't SetResourceBuilder more useful?
There was a problem hiding this comment.
Yea good point. It may still be preferred by some. I've been shoving resource configuration into a method then using it like providerBuilder.ConfigureResource(ConfigureMyResource).
src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/LogRecordExtensions.cs
Outdated
Show resolved
Hide resolved
|
|
||
| this.InstrumentationScope = null; | ||
|
|
||
| this.Attributes = stateValues; |
There was a problem hiding this comment.
nit: rename local stateValues -> attributes
There was a problem hiding this comment.
This ctor our code no longer uses. I'm maintaining it because I know of at least one user calling it to modify log records which were previously immutable. So I left it "stateValues" to be ultra-conservative as far as not breaking anyone's reflection. Unlikely they are looking at parameter names but I figured might as well be safe?
There was a problem hiding this comment.
I'm maintaining it because I know of at least one user calling it to modify log records which were previously immutable.
Hmm, now that they're mutable, maybe we're ok to break them now? If so, probably makes sense to do it in a separate PR for visibility.
| { | ||
| options.AddConsoleExporter(); | ||
| }); | ||
| builder.AddOpenTelemetry().AddConsoleExporter(); |
There was a problem hiding this comment.
Regarding the sample in the PR description:
builder.Logging
.ClearProviders()
.AddEventLog() // Order must be changed
.AddOpenTelemetry(options => options.ParseStateValues = true)
.SetResourceBuilder(resourceBuilder)
.AddConsoleExporter();For the sake of other reviewers, I found it helpful that you pointed out that while it is common for builder methods to be chainable like the Authorization extensions, even ASP.NET Core has examples where the extensions are not chainable like with the Authentication extensions (i.e., does not return the IServiceCollection).
I think this is good precedent that requiring AddOpenTelemetry be at the end is OK.
There was a problem hiding this comment.
Another good example: AddMvc. Returns IMvcBuilder.
|
Going to reopen this against a dedicated branch. |
Fixes #3637
[Apologies for the code bomb! 💣 💥 🤯]
Changes
The OpenTelemetry specification now has API definitions for
LoggerProvider&Logger. This PR adds those to the API project and moves some of what was in SDK into API (LogRecordData+LogRecordAttributeList). What is in API is very faithful to the spec, I didn't bring in any of the ILogger-specific stuff SDK has been using.internal. Only the class definitions arepublicat the moment. I tried to make everything internal but that became way more work than it was worth. Making the classes at least public lets all the standard extensions work.There is now a dedicated
LoggerProviderSdkwhich contains much of whatOpenTelemetryLoggerProvidercontained previously. OurILoggersupport is now more of an integration than a fundamental thing. The entry point to logging is theLoggerProviderand that is fed into theILoggerProviderintegration in the same way it is fed into the Serilog or the EventSource adapter things.All of the extension methods added in [Logs] Support dependency injection in logging build-up #3504 have been moved to
LoggerProviderBuilder. There really isn't a difference between logs, traces, & metrics now. All the options that were specific to the ILogger integration are now separate from the provider.LogEmitteris gone.LoggerAPI now has methods for emitting logs + events. Serilog & EventSource projects now use those.Refactored some of the builder state & service registration helpers so all signals can use them.
Tried to improve the SDK
LogRecordas far as its spec compliance.StateValuesin favor ofAttributes.Bodywhich is now set to{OriginalFormat}automatically for ILogger messages if it is detected as the last element in attributes.Stateby makingAttributesmore useful (see next bullet).TraceStateonLogRecords. That wasn't in the spec. I added an option to turn it back on if desired.State+StateValues+ParseStateValuesThese things evolved to be strange. Exporters are having to check
StateValues&Stateand some are forcingParseStateValues = true.Stateitself is not safe to access beyond the log lifecycle. I tweaked things so there is a more deterministic behavior. IfTStateisIReadOnlyList<KeyValuePair<string, object>>(orIEnumerable<>equivalent) thanLogRecord.Attributeswill now always be populated. That will cover most messages written throughILoggerusing the source generator or the extensions. The only way to pass something that doesn't meet that requirement is callingILogger.Log<T>(...)directly. In that case ifParseStateValues = truethan we will buildAttributesdynamically using reflection.This allows for the deprecation of
LogRecord.State. Exporters should now be able to look atLogRecord.Attributesfor everything and get a nice and consistent behavior.If users don't care for export of attributes at all there is now an option
IncludeStateto turn off all operations againstTState.More coming soon.
Public API Changes
OpenTelemetry.Api
Kept most of the spec surface internal but needed a couple base classes exposed for extensions to bind to:
namespace OpenTelemetry.Logs { + public class LoggerProvider : BaseProvider + { + protected LoggerProvider() {} + } + public abstract class LoggerProviderBuilder + { + protected LoggerProviderBuilder() {} + } }OpenTelemetry
namespace OpenTelemetry.Logs { public sealed class LogRecord { + public string? Body { get; set; } + public IReadOnlyList<KeyValuePair<string, object?>>? Attributes { get; set; } + [Obsolete("State cannot be accessed safely outside of an ILogger.Log call stack. It will be removed in a future version.")] public object? State { get; set; } + [Obsolete("Use Attributes instead StateValues will be removed in a future version.")] public IReadOnlyList<KeyValuePair<string, object?>>? StateValues { get; set; } } public class OpenTelemetryLoggerOptions { + public bool IncludeState { get; set; } = true; + public bool IncludeTraceState { get; set; } + [Obsolete("Use LoggerProviderBuilder instead of OpenTelemetryLoggerOptions to configure a LoggerProvider this method will be removed in a future version.")] public OpenTelemetryLoggerOptions AddProcessor(BaseProcessor<LogRecord> processor) {} + [Obsolete("Use LoggerProviderBuilder instead of OpenTelemetryLoggerOptions to configure a LoggerProvider this method will be removed in a future version.")] public OpenTelemetryLoggerOptions SetResourceBuilder(ResourceBuilder resourceBuilder) {} } + public static class LoggerProviderBuilderExtensions + { // These were recently added on OpenTelemetryLoggerOptions (#3504 #3596) but are now extensions for LoggerProviderBuilder + public static LoggerProviderBuilder SetResourceBuilder(this LoggerProviderBuilder loggerProviderBuilder, ResourceBuilder resourceBuilder) {} + public static LoggerProviderBuilder ConfigureResource(this LoggerProviderBuilder loggerProviderBuilder, Action<ResourceBuilder> configure) {} + public static LoggerProviderBuilder AddProcessor(this LoggerProviderBuilder loggerProviderBuilder, BaseProcessor<LogRecord> processor) {} + public static LoggerProviderBuilder AddProcessor<T>(this LoggerProviderBuilder loggerProviderBuilder) where T : BaseProcessor<LogRecord> {} + public static LoggerProviderBuilder AddExporter(this LoggerProviderBuilder loggerProviderBuilder, ExportProcessorType exportProcessorType, BaseExporter<LogRecord> exporter) {} + public static LoggerProviderBuilder AddExporter(this LoggerProviderBuilder loggerProviderBuilder, ExportProcessorType exportProcessorType, BaseExporter<LogRecord> exporter, Action<ExportLogRecordProcessorOptions> configure) {} + public static LoggerProviderBuilder AddExporter(this LoggerProviderBuilder loggerProviderBuilder, ExportProcessorType exportProcessorType, BaseExporter<LogRecord> exporter, string? name, Action<ExportLogRecordProcessorOptions>? configure) {} + public static LoggerProviderBuilder AddExporter<T>(this LoggerProviderBuilder loggerProviderBuilder, ExportProcessorType exportProcessorType) where T : BaseExporter<LogRecord> {} + public static LoggerProviderBuilder AddExporter<T>(this LoggerProviderBuilder loggerProviderBuilder, ExportProcessorType exportProcessorType, Action<ExportLogRecordProcessorOptions> configure) where T : BaseExporter<LogRecord> {} + public static LoggerProviderBuilder AddExporter<T>(this LoggerProviderBuilder loggerProviderBuilder, ExportProcessorType exportProcessorType, string? name, Action<ExportLogRecordProcessorOptions>? configure) where T : BaseExporter<LogRecord> {} + public static LoggerProviderBuilder ConfigureServices(this LoggerProviderBuilder loggerProviderBuilder, Action<IServiceCollection> configure) {} + public static LoggerProviderBuilder ConfigureBuilder(this LoggerProviderBuilder loggerProviderBuilder, Action<IServiceProvider, LoggerProviderBuilder> configure) {} + public static LoggerProvider Build(this LoggerProviderBuilder loggerProviderBuilder) {} // These are new and replace the OpenTelemetryLoggerOptions.ConfigureProvider method added on #3504 + public static LoggerProviderBuilder AddInstrumentation<T>(this LoggerProviderBuilder loggerProviderBuilder) where T : class {} + public static LoggerProviderBuilder AddInstrumentation<T>(this LoggerProviderBuilder loggerProviderBuilder, Func<IServiceProvider, LoggerProvider, T> instrumentationFactory) where T : class {} + } public static class Sdk { - public static OpenTelemetryLoggerOptions CreateLoggerProviderBuilder() {} // Added on #3504 + public static LoggerProviderBuilder CreateLoggerProviderBuilder() {} } + public static class LoggerProviderBuilderServiceCollectionExtensions + { + public static IServiceCollection ConfigureOpenTelemetryLogging(this IServiceCollection services) {} + public static IServiceCollection ConfigureOpenTelemetryLogging(this IServiceCollection services, Action<LoggerProviderBuilder>? configure) {} + } public static class OpenTelemetryLoggingExtensions { // The return type has change on these. See the section on backwards compatibility - public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder) {} - public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Action<OpenTelemetryLoggerOptions> configureOptions) {} + public static OpenTelemetryLoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder) {} + public static OpenTelemetryLoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Action<OpenTelemetryLoggerOptions> configureOptions) {} // These were changed to accept LoggerProvider instance instead of OpenTelemetryLoggerProvider instance + public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, LoggerProvider loggerProvider) {} + public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, LoggerProvider loggerProvider, Action<OpenTelemetryLoggerOptions> configureOptions) {} + public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, LoggerProvider loggerProvider, Action<OpenTelemetryLoggerOptions>? configureOptions, bool disposeProvider) {} } // See the section on backwards compatibility + public sealed class OpenTelemetryLoggingBuilder : LoggerProviderBuilder, ILoggingBuilder + { + public IServiceCollection Services { get; } // Needed for ILoggingBuilder + } // Added on #3504 but removed. These will come back as extensions on LoggerProvider once spec is stable. public class OpenTelemetryLoggerProvider { - public OpenTelemetryLoggerProvider AddProcessor(BaseProcessor<LogRecord> processor) {} - public bool ForceFlush(int timeoutMilliseconds = -1) {} } // Added on #3504 but removed. Didn't seem to be really needed now that OpenTelemetryLoggerOptions is no longer also the builder thing. public class OpenTelemetryLoggerOptions { - OpenTelemetryLoggerOptions SetIncludeFormattedMessage(bool enabled) {} - OpenTelemetryLoggerOptions SetIncludeScopes(bool enabled) {} - OpenTelemetryLoggerOptions SetParseStateValues(bool enabled) {} } }More coming soon.
Backwards compatibility
The stable API in 1.3.1 uses OpenTelemetryLoggerOptions as the builder for logging. There isn't much to it, only
AddProcessorandSetResourceBuilderare available. Extensions are coded to use that API when building up the provider.This PR is essentially splitting things up so that
LoggingProviderBuilderis responsible for building the specLoggerProviderandOpenTelemetryLoggerOptionsis responsible for configuring theOpenTelemetryLoggerProviderwhich is theILoggerintegration.This was tricky and I spent a lot of time trying many different things. This is the best I could come up with! Open to suggestions 🤔
OpenTelemetryLoggerOptions.AddProcessor&OpenTelemetryLoggerOptions.SetResourceBuilderstill exist. Code and extensions that rely on them will continue to function. How this works is anything set on the options will be applied to the provider when it is ready.LoggingProviderBuilderhas all the great stuff now. This is the API we want to use instead. To encourage migrationOpenTelemetryLoggerOptions.AddProcessor&OpenTelemetryLoggerOptions.SetResourceBuilderare now marked asObsoleteas are the extensions usingOpenTelemetryLoggerOptions. New extensions have been added targetingLoggingProviderBuilderinstead.Here is some working 1.3.1 code:
That will continue to work but now generates warnings due to the obsoletions. Here is the new pattern:
AddOpenTelemetryreturnsOpenTelemetryLoggingBuilderwhich implementsLoggerProviderBuilderto configure the builder.This pattern works equally well:
As does...
The first pattern configures everything off of
ILoggingBuilderin one spot. The second two configureLoggerProviderindependently and then tellILoggerBuilder(ILogger integration) to use it. We don't have to provider a "one-spot" option, but if ILogger integration is our most common scenario it is kind of nice.Alternative ideas
This one feels close:
It makes our pattern...
Why I didn't go with this:
This one is tempting...
...but that has all the pitfalls this PR is trying to avoid.
LoggerProviderBuilderneeds theIServiceCollectionand is invoked immediately.OpenTelemetryLoggerOptionsis part of Options API so it doesn't really exist until theIServiceProvideris ready. To try and combine them you have to do stuff like this. It is best to let builders be builders and let options be options 😄TODOs
CHANGELOG.mdupdated for non-trivial changes