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

RequestLoggingInterceptor logs response time and code #1206

Merged
merged 1 commit into from Sep 24, 2019

Conversation

@mightyguava
Copy link
Collaborator

commented Sep 23, 2019

This implementation isn't ideal. It splits the interceptor into RequestLoggingInterceptor (NetworkInterceptor) and RequestBodyLoggingInterceptor (ApplicationInterceptor).

Request body is not available to a NetworkInterceptor since the body hasn't been decoded yet. Response code is not available to an ApplicationInterceptor since ApplicationInterceptors are actually implemented as the last NetworkInterceptor the request chain (the response chain is reversed), and ExceptionHandlingInterceptor hasn't executed yet.

Splitting means that if request/responses are sampled and includeBody is on, the RequestLoggingInterceptor and RequestBodyLoggingInterceptor do not log on the same set of requests. While not ideal, it should be fine since they are effectively independent.

@mightyguava mightyguava requested review from swankjesse and alecholmes Sep 23, 2019
@mightyguava mightyguava force-pushed the yunchi/code branch from 76be3cd to 0136b74 Sep 24, 2019
Copy link
Collaborator

left a comment

LG, but I'm not really seeing how the new interceptors are logging on different sets of requests. Mind elaborating?

@@ -113,10 +114,14 @@ class MiskWebModule(private val config: WebConfig) : KAbstractModule() {
multibind<NetworkInterceptor.Factory>(MiskDefault::class)
.to<ExceptionHandlingInterceptor.Factory>()

// Optionally log request and response details
multibind<ApplicationInterceptor.Factory>(MiskDefault::class)
// Log request and response details

This comment has been minimized.

Copy link
@wesleyk

wesleyk Sep 24, 2019

Collaborator

isn't this optional as well based on sampling?

This comment has been minimized.

Copy link
@mightyguava

mightyguava Sep 24, 2019

Author Collaborator

I mistakenly thought this was opt-out, but it's opt-in. 👍

@mightyguava

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 24, 2019

LG, but I'm not really seeing how the new interceptors are logging on different sets of requests. Mind elaborating?

The interceptors share the same sampling rate, but independently generate random numbers to determine what is sampled.

@wesleyk

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

The interceptors share the same sampling rate, but independently generate random numbers to determine what is sampled.

mm gotcha, so there could be overlap. Thanks for clarifying!

@mightyguava mightyguava force-pushed the yunchi/code branch from 0136b74 to 22b4f26 Sep 24, 2019
@mightyguava mightyguava merged commit 3040c68 into master Sep 24, 2019
3 checks passed
3 checks passed
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: java Your tests passed on CircleCI!
Details
ci/circleci: node Your tests passed on CircleCI!
Details
@mightyguava mightyguava deleted the yunchi/code branch Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.