rybalkinsd / kohttp

Kotlin DSL http client
https://kohttp.gitbook.io
Apache License 2.0
479 stars 42 forks source link

Log Interceptor #64

Closed rybalkinsd closed 5 years ago

rybalkinsd commented 5 years ago

Analyse okhttp log interceptor Figure out how to provide our own with minimal DSL configuration Try to avoid any dependencies for this case.

From the first view okhttp-logging-interceptor's provides too verbose logs

gokulchandra commented 5 years ago

@rybalkinsd Is the intent to provide an interceptor for logging which can be configured by using a DSL?

Should we discuss a high level design of how it could possibly look?

rybalkinsd commented 5 years ago

@gokulchandra yeah, better to discuss here. marked task as design stage

cc @shtykh

rybalkinsd commented 5 years ago

At first, there are problems with fork { } and interceptors in current implementation:

  1. client Interceptors replaced (not inherited) by fork
  2. dsl looks rather ugly
    client. fork {
    interceptors = listOf(A, B, C)
    }

    putting both together, in my mind html dsl could be a good reference for us.

client. fork {
    interceptpors {
        +A
        +B
        +C
    }
}

but what's our other options?

rybalkinsd commented 5 years ago

Also, as Interceptor is SAM interface, it's possible to go with

client. fork {
    interceptors {
        + Interceptor { chain -> Response() }
    }
}

About the logger: currently the project does not have any logger in deps. Okhttp even provides a separate Logger interface.

On of the possible definitions: Log(appender: String -> Unit)

client. fork {
    interceptors {
        // Slf4j
        +Log { LoggerFactory.getLogger("logger name")::info }
        // console
        +Log { ::println }
    }
}
rybalkinsd commented 5 years ago

@gokulchandra Any thoughts about the design?

gokulchandra commented 5 years ago

@rybalkinsd Sorry for taking my time.

It sounds like a good approach. I do have a few questions:

rybalkinsd commented 5 years ago

From general thought client fork is similar to decorator pattern. This way we’re taking I1, I2, I3 from client and adding I0 that would wrap the chain of I1, I2, I3 (interceptor can act both before and after chain.proceed() call)

If we’ll go without unar plus operator - how we can design add action?

About helper methods - I suggest to start without, make them immutable (read only) and check clients thoughts later

About logging - what exactly it could be?

gokulchandra commented 5 years ago

If we’ll go without unary plus operator - how we can design add action? You're right. I was thinking if there was a way we could just infer the interceptors in the DSL and append them.

About helper methods - I suggest to start without, make them immutable (read only) and check clients thoughts later

👍

About logging - what exactly it could be?

I thought we should allow the user to exclude:

But I guess these are nice to have rather than a part of the core function. I have an initial thought on the Interceptors DSL, I'll be able to push a PR for it later tonight or tomorrow.

rybalkinsd commented 5 years ago

toggle logging headers exclude certain urls/methods

sounds reasonable, let's plan as a separate issue, since it looks more like an enhancement