Adding HttpLogOptions for Logging improvements #5688

samvaity · 2019-10-04T01:22:19Z

Reference #4154

...pconfiguration/src/main/java/com/azure/data/appconfiguration/ConfigurationClientBuilder.java

...onfiguration/src/test/java/com/azure/data/appconfiguration/ConfigurationAsyncClientTest.java

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLogOptions.java

JonathanGiles · 2019-10-04T03:10:17Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLogDetailLevel.java

+    /**
+     * Logs everything in QUERY PARAMS.
+     */
+    QUERY_PARAMS;


This doesn't quite make sense - if the user opts in for QUERY_PARAMS, that means that they can't also get BODY or BODY_AND_HEADERS.

so should we keep the requirement to log query params on only BODY or BODY_AND_HEADERS or both?

BASIC indicates log url (that includes query parameter) and skip headers & body. Wondering why we need QUERY_PARAMS enum option for a specific part of url, Does guideline enforce it?

I don't think the guideline enforces to have the QUERY_PARAMS enum. It can be removed, but still, not sure if we need BODY_AND_HEADERS with it.

Yes, remove QUERY_PARAMS. Leave the rest as-is I would think.

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java

anuchandy · 2019-10-04T15:28:13Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java

@@ -115,7 +120,40 @@ public HttpLoggingPolicy(HttpLogDetailLevel detailLevel, boolean prettyPrintJSON
        return reqBodyLoggingMono;
    }

-    private Function<HttpResponse, Mono<HttpResponse>> logResponseDelegate(final ClientLogger logger, final URL url,
+    private void formatAllowableHeaders(Set<String> allowedHeaderNames, HttpRequest request, ClientLogger logger) {


@samvaity / @JonathanGiles does guideline proposes different meaning for empty vs null value for allowedHeaderNames? i.e. something like empty set means redact all headers and null means log all headers?

I was also thinking if we should add something like a "*", to say log all headers maybe..

At present null and empty set should mean the same thing - to not print any headers.

JonathanGiles

Looks good. Just a few minor things to consider.

JonathanGiles · 2019-10-06T23:27:25Z

...pconfiguration/src/main/java/com/azure/data/appconfiguration/ConfigurationClientBuilder.java

        policies = new ArrayList<>();
+        httpLogOptions = new HttpLogOptions().setLogLevel(HttpLogDetailLevel.NONE);


It probably makes sense to just have the log level be NONE by default.

JonathanGiles · 2019-10-06T23:28:40Z

...pconfiguration/src/main/java/com/azure/data/appconfiguration/ConfigurationClientBuilder.java

-    public ConfigurationClientBuilder httpLogDetailLevel(HttpLogDetailLevel logLevel) {
-        httpLogDetailLevel = Objects.requireNonNull(logLevel);
+    public ConfigurationClientBuilder httpLogOptions(HttpLogOptions logOptions) {
+        httpLogOptions = Objects.requireNonNull(logOptions, "Http log options cannot be null.");


We should probably allow null now that I think about it - this lets the user clear out the configuration in the builder so that the next time they call build, they will get an HttpLoggingPolicy with null options. This also means you need to have HttpLoggingPolicy know what to do if the options are null!

JonathanGiles · 2019-10-06T23:35:19Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLogOptions.java

+     * @throws NullPointerException If {@code logLevel} is {@code null}.
+     */
+    public HttpLogOptions setLogLevel(final HttpLogDetailLevel logLevel) {
+        this.logLevel = Objects.requireNonNull(logLevel);


Maybe rather than require non-null, if null is passed in just set it to NONE. Be sure to document that though.

JonathanGiles · 2019-10-06T23:37:09Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java

     */
-    public HttpLoggingPolicy(HttpLogDetailLevel detailLevel, boolean prettyPrintJSON) {
-        this.detailLevel = detailLevel;
+    public HttpLoggingPolicy(HttpLogOptions httpLogOptions, boolean prettyPrintJSON) {


I'm curious if, outside of unit tests, anyone calls this constructor. If no one does, I would like to see this constructor made package-private for now, because this kind of boolean config should be in the HttpLogOptions object.

JonathanGiles · 2019-10-06T23:38:59Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java

        //
        Mono<Void> reqBodyLoggingMono = Mono.empty();
        //
-        if (detailLevel.shouldLogBody()) {
+        if (httpLogOptions.getLogLevel().shouldLogBody()) {


Whenever I see a method call (like getLogLevel()) multiple times I prefer to see it referred to in a local variable.

JonathanGiles · 2019-10-06T23:41:15Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java

+                sb.append(REDACTED_PLACEHOLDER);
+            }
+        }
+        logger.info(sb.toString());


Should there be a newline or whitespace included in this concatenation operation?

JonathanGiles · 2019-10-06T23:43:00Z

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java

                }
+                logger.info(sb.toString());


This header concatenation code should be centralised in this class rather than duplicated for request and response headers.

samvaity requested review from alzimmermsft, conniey, hemanttanwar, jianghaolu, mssfang, sima-zhu and srnagar as code owners October 4, 2019 01:22

samvaity self-assigned this Oct 4, 2019

samvaity requested a review from joshfree October 4, 2019 01:22

JonathanGiles reviewed Oct 4, 2019

View reviewed changes

anuchandy reviewed Oct 4, 2019

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java Show resolved Hide resolved

anuchandy reviewed Oct 4, 2019

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java Outdated Show resolved Hide resolved

anuchandy reviewed Oct 4, 2019

View reviewed changes

sdk/core/azure-core/src/main/java/com/azure/core/http/policy/HttpLoggingPolicy.java Outdated Show resolved Hide resolved

anuchandy reviewed Oct 4, 2019

View reviewed changes

samvaity requested review from g2vinay, gapra-msft, jaschrep-msft and rickle-msft as code owners October 4, 2019 19:22

samvaity added 3 commits October 4, 2019 12:22

adding http log options

aebfa16

review comments

e8721a8

adding httpLogOptions to all clients

54cad07

samvaity force-pushed the logging-redaction branch 3 times, most recently from ff415ac to 2feba11 Compare October 4, 2019 20:17

fix storage tests

9034f2a

samvaity force-pushed the logging-redaction branch from 9d028c7 to 9034f2a Compare October 4, 2019 21:55

JonathanGiles requested changes Oct 6, 2019

View reviewed changes

samvaity added 2 commits October 7, 2019 11:10

review changes

84edfcf

Merge remote-tracking branch 'upstream/master' into logging-redaction

15f6b41

JonathanGiles approved these changes Oct 7, 2019

View reviewed changes

check if shouldLogHeaders

9f0bbb0

samvaity merged commit c0c86cd into Azure:master Oct 7, 2019

samvaity mentioned this pull request Oct 10, 2019

Add Default Logging and Redactions of Logs #4328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding HttpLogOptions for Logging improvements #5688

Adding HttpLogOptions for Logging improvements #5688

samvaity commented Oct 4, 2019

JonathanGiles Oct 4, 2019

samvaity Oct 4, 2019

anuchandy Oct 4, 2019

samvaity Oct 4, 2019

JonathanGiles Oct 6, 2019

anuchandy Oct 4, 2019

samvaity Oct 4, 2019 •

edited

Loading

JonathanGiles Oct 6, 2019

JonathanGiles left a comment

JonathanGiles Oct 6, 2019

JonathanGiles Oct 6, 2019

JonathanGiles Oct 6, 2019

JonathanGiles Oct 6, 2019

JonathanGiles Oct 6, 2019

JonathanGiles Oct 6, 2019

JonathanGiles Oct 6, 2019

		policies = new ArrayList<>();
		httpLogOptions = new HttpLogOptions().setLogLevel(HttpLogDetailLevel.NONE);

Adding HttpLogOptions for Logging improvements #5688

Adding HttpLogOptions for Logging improvements #5688

Conversation

samvaity commented Oct 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samvaity Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanGiles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samvaity Oct 4, 2019 •

edited

Loading