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

Structured logging with ClientLogger #25247

Merged
merged 16 commits into from
Nov 25, 2021
Merged

Conversation

lmolkova
Copy link
Member

@lmolkova lmolkova commented Nov 8, 2021

Description: https://gist.github.com/lmolkova/04f6484595fdd7edded9f4d82619291f

Inspired by SLF4J v2 API

Usage

// todo: overloads for atInfo, etc
logger.atLevel(LogLevel.WARNING)
                .addKeyValue("connectionId", "foo")
                .addKeyValue("linkName", "bar")
                .log("hello, argument = {}", arg)

Outcome:
2021-11-08 15:04:42.407 [ForkJoinPool-1-worker-3] [WARN] com.azure.core.util.logging.ClientLoggerTests - hello, argument = 1, az.sdk.context={"connectionId":"foo", "linkName":"bar"}

Micro-benchmarks:

Benchmark                                           Mode  Cnt     Score     Error  Units
LoggingBenchmark.loggingAtDisabledLevel             avgt   15     1.268 ±   0.153  ns/op
LoggingBenchmark.loggingAtDisabledLevelWithContext  avgt   15     1.524 ±   0.092  ns/op
LoggingBenchmark.loggingAtDisabledLevelWithMDC      avgt   15     1.588 ±   0.252  ns/op
LoggingBenchmark.loggingAtEnabledLevel              avgt   15  2501.731 ± 549.315  ns/op
LoggingBenchmark.loggingAtEnabledLevelWithContext   avgt   15  2711.800 ± 259.968  ns/op
LoggingBenchmark.loggingAtEnabledLevelWithMDC       avgt   15  3624.982 ± 193.837  ns/op

Fluent pattern (loggingAtEnabledLevelWithContext) vs explicit context (loggingAtEnabledLevelWithContext2) don't show significant difference in perf:

Benchmark                                           Mode  Cnt     Score     Error  Units
LoggingBenchmark.loggingAtEnabledLevel              avgt   15  1847.670 ± 128.052  ns/op
LoggingBenchmark.loggingAtEnabledLevelWithContext   avgt   15  2115.740 ± 134.049  ns/op
LoggingBenchmark.loggingAtEnabledLevelWithContext2  avgt   15  2106.112 ±  79.263  ns/op

@ghost ghost added the Azure.Core azure-core label Nov 8, 2021
@Azure Azure deleted a comment from azure-sdk Nov 9, 2021
@azure-sdk
Copy link
Collaborator

API changes have been detected in com.azure:azure-core. You can review API changes here

1 similar comment
@azure-sdk
Copy link
Collaborator

API changes have been detected in com.azure:azure-core. You can review API changes here

@lmolkova lmolkova force-pushed the logging-api-context branch from 8307552 to a098062 Compare November 13, 2021 05:51
@azure-sdk
Copy link
Collaborator

API changes have been detected in com.azure:azure-core. You can review API changes here

@azure-sdk
Copy link
Collaborator

API changes have been detected in com.azure:azure-core. You can review API changes here

@pallavit
Copy link
Contributor

With this change we now support 2 ways to log (Fluent vs non-fluent) - should we long term consolidate to only one style?

@JonathanGiles
Copy link
Member

With this change we now support 2 ways to log (Fluent vs non-fluent) - should we long term consolidate to only one style?

It's possible we could do this, but in terms of backwards compatibility, I wouldn't worry too much. This is exactly the pattern SLF4J is taking in its 2.0.0 release.

Copy link
Member

@JonathanGiles JonathanGiles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really promising!

@pallavit
Copy link
Contributor

With this change we now support 2 ways to log (Fluent vs non-fluent) - should we long term consolidate to only one style?

It's possible we could do this, but in terms of backwards compatibility, I wouldn't worry too much. This is exactly the pattern SLF4J is taking in its 2.0.0 release.

I meant should we think of only exposing Fluent way of doing things moving forward (whenever we think it is the right time) ? I understand our ecosystem supports multiple ways of doing the same thing but shouldn't our SDKs have a single way of doing everything.

@pallavit
Copy link
Contributor

@lmolkova this is pretty cool. I love how you are adding benchmarks to the PRs.

@lmolkova lmolkova changed the title Structured logging: proof of concept Structured logging on ClientLoggger Nov 17, 2021
@lmolkova lmolkova changed the title Structured logging on ClientLoggger Structured logging on ClientLogger Nov 17, 2021
@lmolkova
Copy link
Member Author

I meant should we think of only exposing Fluent way of doing things moving forward (whenever we think it is the right time) ?

Fluent way is a bit less performant (you have to allocate the builder), a bit more verbose (log.atInfo().log("foo") vs log.info("foo")) and it feels like it should only be used when we'd want to add context. I also believe ClientLogger, even though it's public, is only used by our own SDKs and de-facto is an internal API. It's used widely (e.g. there are ~6k calls to it in our repo).
So, I didn't have plans to ever remove flat APIs.

@lmolkova lmolkova changed the title Structured logging on ClientLogger Structured logging with ClientLogger Nov 17, 2021
@lmolkova lmolkova force-pushed the logging-api-context branch from c48d9c3 to 6a2d3df Compare November 17, 2021 06:46
@lmolkova lmolkova requested review from yiliuTo and a team as code owners November 24, 2021 21:06
@lmolkova lmolkova force-pushed the logging-api-context branch from a19ceda to 00d00b3 Compare November 24, 2021 21:10
@lmolkova lmolkova force-pushed the logging-api-context branch from 00d00b3 to fc7aa8f Compare November 24, 2021 21:37
@lmolkova
Copy link
Member Author

lmolkova commented Nov 24, 2021

Fresh benchmark, ~same results

Benchmark                                           Mode  Cnt     Score     Error  Units
LoggingBenchmark.loggingAtDisabledLevel             avgt    6     1.511 ±   0.311  ns/op
LoggingBenchmark.loggingAtDisabledLevelWithContext  avgt    6     1.693 ±   0.433  ns/op
LoggingBenchmark.loggingAtEnabledLevel              avgt    6  1309.170 ± 414.924  ns/op
LoggingBenchmark.loggingAtEnabledLevelWithContext   avgt    6  1546.085 ± 890.383  ns/op

@lmolkova lmolkova enabled auto-merge (squash) November 24, 2021 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core azure-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants