-
Notifications
You must be signed in to change notification settings - Fork 762
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
Introduce new extended logging model. #4110
Conversation
c9ebcd5
to
dccda2d
Compare
Second round:
|
dccda2d
to
b3f3cd4
Compare
This looks much cleaner than what's currently there and its marked experimental. Are you going to measure the overhead? |
src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Logging/LoggerExtensions.cs
Outdated
Show resolved
Hide resolved
b3f3cd4
to
f3d8522
Compare
Third round:
|
src/Libraries/Microsoft.Extensions.Telemetry/Logging/ExtendedLoggingExtensions.cs
Outdated
Show resolved
Hide resolved
a25f03b
to
d4ce094
Compare
@noahfalk @dpk83 The PR is now in pretty good shape. I have some basic tests that show the machinery is generally working. I'll get to 100% coverage Monday. Please see the few outstanding notes in the description above. Relative to the original R9 solution, I expect this to be faster as we now have zero-copy static log enrichment, and I've replaced pools with faster thread-local storage instead for working state. All calls to ILogger.Log now involve going through an additional ILogger instance than before, so we pay for a new interface call. A quick way to eliminate the overhead is to take the logic that's in the default logger from Microsoft.Extensions.Logging and embed directly into the new Logger. We wouldn't need the indirection that way, at the cost of cut & paste of logic. |
d4ce094
to
c5f88cc
Compare
src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Enrichment/EnricherExtensions.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry.Abstractions/Enrichment/EnricherExtensions.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Logging/ExtendedLoggerFactory.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.Telemetry/Logging/ExtendedLoggerFactory.cs
Outdated
Show resolved
Hide resolved
c5f88cc
to
c5af187
Compare
e5f9437
to
6f72235
Compare
#if NET5_0_OR_GREATER | ||
var pid = Environment.ProcessId; | ||
#else | ||
var pid = System.Diagnostics.Process.GetCurrentProcess().Id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't System.Diagnostics.Process.GetCurrentProcess()
be disposed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC it allocates some unmanaged resources
{ | ||
try | ||
{ | ||
enricher(joiner.PropertyBag); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geeknoid what if we provide here an additional information/context to an enricher? TState
from ILogger.Log<TState>()
maybe.
Otherwise IStaticLogEnricher
is not that different from ILogEnricher
, apart from using some fancy things like AsyncContext
.
cc @andrey-noskov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the enricher do with the tstate? I'm fine with the idea of doing this, so long as we have a use case to justify it.
d17136c
to
eafbc67
Compare
- Replace the OpenTelemetry-specific logger design with a LoggerFactory-based approach independent of OpenTelemetry. This delivers enrichment and redaction to all ILogger users. The basic idea is that the code generator will no longer be responsible for doing redaction. Instead, the generated code will accumulate normal properties in one collection and will accumulate classified properties in a different collection. Within the Logger type, the classified properties are run through redaction and added to the normal property list. This list is then used to enrich into. And then the final thing is given to the set of currently registered ILogger instances in the system. So all these loggers get redacted and enriched state. This separates static vs. dynamic log enrichment. The idea is to reduce overhead for stuff that never changes. Thus, we have the new IStaticLogEnricher type. We now support structured logging as a first class concept using the new StructuredLog extension methods around ILogger. NOTES: - The idea is that the customer should call AddExtendedLogging instead of AddLogging. By virtue of using this call, they'll also be using the newer code generator which will take advantage of the new features. - This PR doesn't currently include an updated logging code generator that will take advantage of this new functionality. - Right now, if a user calls AddLogging after they had already called AddExtendedLogging, this will undo the extended logging stuff and leave the system in classic mode. We need to find a strategy to deal with this.
eafbc67
to
6b963dc
Compare
Introduce new extended logging model.
LoggerFactory-based approach independent of OpenTelemetry. This
delivers enrichment and redaction to all ILogger users.
The basic idea is that the code generator will no longer be responsible
for doing redaction. Instead, the generated code will accumulate
normal properties in one collection and will accumulate classified
properties in a different collection.
Within the Logger type, the classified properties are run through
redaction and added to the normal property list. This list is then
used to enrich into. And then the final thing is given to the set of
currently registered ILogger instances in the system. So all these
loggers get redacted and enriched state.
This separates static vs. dynamic log enrichment. The idea is to
reduce overhead for stuff that never changes. Thus, we have the new
IStaticLogEnricher type.
NOTES:
that will take advantage of this new functionality.
Microsoft Reviewers: Open in CodeFlow