-
Notifications
You must be signed in to change notification settings - Fork 9.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
Add a LoggingEventListener and use it in okcurl #4353
Conversation
There seems to be inconsistency between javdoc parsing between 'mvn verify' and Travis CI. Before the change, 'mvn clean verify' passes but Travis CI fails due to missing import of okhttp3.OkHttpClient. Just adding the missing import, causes 'mvn verify' to fail die to unused import. Changing the line wrapping seems to appease 'mvn verify'.
logger.log("[t=" + timeMs + "] " + message); | ||
} | ||
|
||
public interface Logger { |
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.
Should we try to bridge HttpLoggingInterceptor.Logger somehow? Seems strange to have two. Presumably over time there will be a complete set of each.
Maybe even introduce as a public API? It seems like something we should encourage e.g. someone implementing a Interceptor should log via Platform.log, but as that is internal API, a way to achieve that correctly would be nice.
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.
You're right. It does introduce duplication. I thought it would be confusing to pass a HttpLoggingInterceptor.Logger
to the LoggingEventListener.Factory
constructor. I can make this change if you think it's better.
If by a public API you mean making Logger
a top-level class, I can do that. To avoid a breaking API change, we then may need to keep HttpLoggingInterceptor(HttpLoggingInterceptor.Logger)
as a deprecated constructor in addition to a new HttpLoggingInterceptor(Logger)
constructor.
Let me know what you feel would be best here.
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.
I suggest sitting for a day in case there are more voices
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.
Since this is in the same package as the HttpLoggingInterceptor, think it’s kosher to just use HttpLoggingInterceptor.Logger directly. Fewer types is better.
This looks like a fantastic first version to me. |
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.
Agreed, this is fantastic. If you can combine the two loggers into one, I’d love to merge this and ship it in our next release.
Sorry for the slow response.
okhttp-logging-interceptor/src/main/java/okhttp3/logging/LoggingEventListener.java
Outdated
Show resolved
Hide resolved
logger.log("[t=" + timeMs + "] " + message); | ||
} | ||
|
||
public interface Logger { |
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.
Since this is in the same package as the HttpLoggingInterceptor, think it’s kosher to just use HttpLoggingInterceptor.Logger directly. Fewer types is better.
okhttp-logging-interceptor/src/main/java/okhttp3/logging/LoggingEventListener.java
Outdated
Show resolved
Hide resolved
okhttp-logging-interceptor/src/test/java/okhttp3/logging/LoggingEventListenerTest.java
Outdated
Show resolved
Hide resolved
okhttp-logging-interceptor/src/main/java/okhttp3/logging/LoggingEventListener.java
Outdated
Show resolved
Hide resolved
okhttp-logging-interceptor/src/test/java/okhttp3/logging/LoggingEventListenerTest.java
Show resolved
Hide resolved
okhttp-logging-interceptor/src/test/java/okhttp3/logging/LoggingEventListenerTest.java
Show resolved
Hide resolved
okhttp-logging-interceptor/src/test/java/okhttp3/logging/LoggingEventListenerTest.java
Outdated
Show resolved
Hide resolved
@swankjesse, should we add a top-level |
Nope. Let's keep the current API. The cost of migrating this is high and the benefit is low. |
😍 |
@yschimke, @swankjesse, will you be receptive to a PR that adds logging of request/response headers and a PR that adds |
I think headers logging is probably too much for this thing. This is for observing timing; the logging interceptor is for debugging content. |
@swankjesse I think content vs timing and connections. So handshake seems useful here. But I agree about not including variable output like headers, request/response body. |
* commit '1f7e796e6e658df34a98276b2092a81de118937d': Cleanup HttpLoggingInterceptor (square#4346) Add a LoggingEventListener and use it in okcurl (square#4353) Preemptive auth for proxy CONNECT Relax handling of Cache-Control: immutable Add some docs for Cache class (square#4375) Fix connection leaks on failed web socket upgrades. Don't specify a crypto provider in HeldCertificate. Confirm that call timeouts don't apply to SSE or web sockets. Add APIs to configure the client's default call timeout. (square#4369) Recover from executor shutdowns gracefully. (square#4365) Make the nested BasicAuthInterceptor static (square#4368) Add basic auth interceptor recipe (square#4336) Whole operation timeouts Make scheme names case-sensitive again. Remove colon when scheme missing in builder toString (square#4361) CipherSuite init speedup (square#4340) Limit the use of regexes in the RFC 7235 challenge parser. Allow incomplete url builder toString usage (square#4357) APIs to set date headers Upgrade Conscrypt to 1.4.0 (was 1.3.0)
A basic logger, as discussed in #4315.
To install a
LoggingEventListener
that uses the platform logger:The user can also pass a custom Logger:
Sample output: