-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Per-instance logging context #25681
Per-instance logging context #25681
Conversation
API changes have been detected in API changes + public ClientLogger(Class<?> clazz, Map<String, Object> context)
+ public ClientLogger(String className, Map<String, Object> context)
+ public LoggingEventBuilder addKeyValue(String key, Object value)
+ public RuntimeException log(RuntimeException runtimeException)
+ public void log(Supplier<String> messageSupplier, Throwable throwable) |
sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java
Outdated
Show resolved
Hide resolved
sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java
Outdated
Show resolved
Hide resolved
bbe649a
to
ea99467
Compare
* @param className Class name creating the logger. | ||
* @param context Context to be populated on every log record written with this logger. | ||
* Objects are serialized with {@code toString()} method. | ||
* @throws RuntimeException in case of invalid arguments. |
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.
Do we throw any other RuntimeException apart from NullPointerException
? We should declare the specific exception types in the @throws
clause.
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.
we are throwing something in DefaultLogger
impl, but realistically we can't document closed set of exceptions here because they depend on SLF4J implementation used in the app.
sdk/core/azure-core/src/main/java/com/azure/core/util/logging/ClientLogger.java
Outdated
Show resolved
Hide resolved
* @return The passed {@link RuntimeException}. | ||
* @throws NullPointerException If {@code runtimeException} is {@code null}. | ||
*/ | ||
public RuntimeException log(RuntimeException runtimeException) { |
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.
Do we need this overload since we already have log(Throwable throwable)
method?
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.
yes, we need it for similar reasons we have one on ClientLogger
: when we do throw log(RuntimeException)
we don't have to add checked exception or cast the exception type.
* @param value Object value. | ||
* @return The updated {@code LoggingEventBuilder} object. | ||
*/ | ||
public LoggingEventBuilder addKeyValue(String key, Object value) { |
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.
We initially only supported primitive types. With Object
, using toString
may not always log a human-friendly message which then requires supporting serializers. Should we add this overload only when required? If something like that is required, the caller can always call toString()
themselves.
Thoughts?
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.
good point. What I found while adopting amqp-core
is that I have to repeatedly write something like
logger.canLogAtLevel(level) {
builder.addKeyValue("foo", value == null ? null : value.toString());
}
and even then it won't compile becasue of ambiguity of overload matching for value == null ? null : value.toString()
So based on this, I realized this overload is essential. I tried to document that we only call toString
everywhere. SLF4J does the same with {}
anchor, i.e. users would have done the same with the old API.
Also, if someone would pass something that doesn't override toString, they would see an unreadable log message and fix it, but it doesn't lead to errors (while explicit toString() might unless it's guarded with all above checks)
2ce0c2a
to
ca5b2e5
Compare
ca5b2e5
to
85414e5
Compare
@JonathanGiles @srnagar can you please take another look? |
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.
LGTM!
[Hub Generated] Review request for Microsoft.PlaywrightTesting to add version preview/2023-10-01-preview (Azure#25445) * Generate new Service for playwrighttesting * Adding Management APIs for Microsoft.AzurePlaywrightService public preview api-version 2023-10-01-preview * CI failures Fix * Removed description form properties as it's not required * Added new resource type * Spell check fix * Renamed quota resource type * Rectifying the typespec issue for location based resource * Renamed enum value * Review comments addressed * Swagger build errors resolution * Swagger model validation error fix for example * Added dashboard URI in examples * Revert "Added dashboard URI in examples" This reverts commit 8b1a7cb8964592d026b176380afea398317f2387. * Mjmadhu playwrighttesting microsoft.azure playwright service 2023 10 01 preview data plane (Azure#25681) * Renamed entity from access-key to access-token * Added subscription state to handle appropriate action for clients * Added account state as a property * Updated regex for names * Updated example * Added readonly properties in PUT request examples
While working on #25671 I found a few gaps not covered in structured logging yet:
connectionId
and sometimesentityPath
orlinkName
.Object
values (with deferredtoString()
call to serialize them)log(RuntimeException)
log(Supplier<String> message, Throwable)
This PR fills these gaps.
I'm also entertaining the idea of separating context-aware logging to a new
ContextualLogger
class to reduce some noise and have clearer expectations for each logger. LMK what you think