feature/enable-custom-http-logger-interceptor #23

ennoeller · 2024-08-30T13:36:21Z

No description provided.

… feature/enable-custom-http-logger-interceptor

rammrain · 2024-09-02T11:19:24Z

Let's take the opportunity to replace okhttp3.logging.HttpLoggingInterceptor with our own implementation that will fix the most common flaws with the interceptor provided by Retrofit:

Add the ability to extend and modify the behaviour of logging
Use fewer logging entries to log the entire request-response cycle
Use MDC to improve the request-response log filtering in Graylog and other platforms

Otherwise the new interceptor should follow the same features as Retrofit has, including log levels and header suppression.

Let's try and fit the entire log in a single log entry.

… feature/enable-custom-http-logger-interceptor

Jorich · 2024-10-10T06:28:07Z

src/main/java/ee/bitweb/core/retrofit/logging/RetrofitLoggingInterceptor.java

+
+            sb.append(getRequestDescription(request, connection));
+
+            if (loggingLevel.getLevel() >= 2) {


For readability reasons I would've written like this
if (loggingLevel.getLevel() >= LoggingLevel.HEADERS.getLevel()) ...
It would've been even better to write something like this
if (loggingLevel.includes(LoggingLevel.HEADERS))

Jorich · 2024-10-10T06:42:27Z

src/main/java/ee/bitweb/core/retrofit/builder/RetrofitApiBuilder.java

    }

-    private OkHttpClient.Builder createDefaultBuilder(HttpLoggingInterceptor loggingInterceptor) {
+    private OkHttpClient.Builder createDefaultBuilder(Interceptor loggingInterceptor) {


Probably should be LoggingInterceptor otherwise someone might misuse it.

Jorich · 2024-10-10T06:47:05Z

src/main/java/ee/bitweb/core/retrofit/logging/RetrofitLoggingInterceptor.java

+                sb.append("\n").append(getRequestHeaders(request));
+            }
+
+            if (loggingLevel.getLevel() >= 3) {


Same here
if (loggingLevel.getLevel() >= LoggingLevel.BODY.getLevel()) ...
It would've been even better to write something like this
if (loggingLevel.includes(LoggingLevel.BODY))

Jorich · 2024-10-10T06:50:08Z

src/main/java/ee/bitweb/core/retrofit/logging/RetrofitLoggingInterceptor.java

+
+        for (int i = 0; i < requestHeaders.size(); i++) {
+            var name = requestHeaders.name(i);
+            if (List.of("content-type", "content-length").contains(name.toLowerCase())) {


In some places we use constants, others we hardcode. Maybe we should follow same convention - use constants

Jorich · 2024-10-10T06:50:34Z

src/main/java/ee/bitweb/core/retrofit/logging/RetrofitLoggingInterceptor.java

+
+        for (int i = 0; i < responseHeaders.size(); i++) {
+            var name = responseHeaders.name(i);
+            if (List.of("content-type", "content-length").contains(name.toLowerCase())) {


In some places we use constants, others we hardcode. Maybe we should follow same convention - use constants

Jorich · 2024-10-10T06:52:56Z

src/main/java/ee/bitweb/core/retrofit/logging/RetrofitLoggingInterceptor.java

+            }
+
+            if (contentLength != 0L) {
+                return buffer.clone().readString(charset);


Maybe we should allow to limit how much of the content gets logged. If response is a 150k element json list of objects, this log entry could cause harm to log collector and become virtually unreadable.

Jorich · 2024-10-10T07:00:46Z

src/main/java/ee/bitweb/core/retrofit/logging/RetrofitLoggingInterceptor.java

+import static okhttp3.internal.http.HttpHeaders.promisesBody;
+
+@Slf4j
+public class RetrofitLoggingInterceptor implements LoggingInterceptor {


We might want to add some sort of white/blacklist logic to control for which endpoints we want to log/omit bodies.
This would allow for example to omit response bodies for requests that contain confidential data in body and handle that separately.

rammrain · 2024-10-10T07:28:12Z

Logging level documentation sample: https://github.com/square/okhttp/blob/master/okhttp-logging-interceptor/src/main/kotlin/okhttp3/logging/HttpLoggingInterceptor.kt#L111

… feature/enable-custom-http-logger-interceptor

Jorich

Mostly looks promising. Just a few small fixes

Jorich · 2024-10-30T13:45:51Z

src/main/java/ee/bitweb/core/retrofit/builder/RetrofitApiBuilder.java

    private Converter.Factory converterFactory;
    private OkHttpClient.Builder clientBuilder;

-    public static <T> RetrofitApiBuilder<T>  create(String baseUrl, Class<T> definition) {


Here goes the backward compatibility :)
I am not sure how many cases use retrofit api builder directly but I am afraid this change is a major version bump.

Jorich · 2024-10-30T13:52:32Z

src/main/java/ee/bitweb/core/retrofit/builder/RetrofitApiBuilder.java

        return this;
    }

-    public RetrofitApiBuilder<T> loggingLevel(LoggingLevel level) {


This also results in major version bump

Jorich · 2024-10-31T06:29:56Z

src/main/java/ee/bitweb/core/retrofit/logging/writers/RetrofitLogLoggerWriterAdapter.java

+    public void write(Map<String, String> container) {
+        Map<String, String> currentContext = MDC.getCopyOfContextMap();
+
+        log(container);


I would wrap this in try {} finally {} block so that MDC wouldn't be left unswapped

Remove some non-standard mappers Add no-op interceptor for when logging has been turned off All mappers are extendable and mapper beans can be overridden

Add tests

Jorich · 2024-11-05T09:23:45Z

src/main/java/ee/bitweb/core/retrofit/RetrofitAutoConfiguration.java

+        return new RetrofitLoggingInterceptorImplementation(mappers, new RetrofitLogLoggerWriterAdapter());
+    }
+
+    // testida üle, kui on vajadus mapper üle kirjutada


I suppose that is done

…r-interceptor' into feature/enable-custom-http-logger-interceptor

sonarqubecloud · 2024-11-05T09:59:06Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

ennoeller added 3 commits August 30, 2024 16:33

feature/enable-custom-http-logger-interceptor

9322d98

feature/enable-custom-http-logger-interceptor

32d25fe

Merge branch 'master' of https://github.com/BitWeb/java-core-lib into…

3bf2f79

… feature/enable-custom-http-logger-interceptor

ennoeller requested a review from rammrain September 2, 2024 06:11

rammrain assigned ennoeller Sep 2, 2024

rammrain marked this pull request as draft September 2, 2024 11:11

rammrain requested a review from Jorich September 2, 2024 11:19

ennoeller added 3 commits October 1, 2024 12:23

Merge branch 'master' of https://github.com/BitWeb/java-core-lib into…

9adea42

… feature/enable-custom-http-logger-interceptor

feature/enable-custom-http-logger-interceptor

d0f28bf

feature/enable-custom-http-logger-interceptor

f18294c

Jorich requested changes Oct 10, 2024

View reviewed changes

ennoeller added 4 commits October 11, 2024 12:20

feature/enable-custom-http-logger-interceptor

d3f7dd8

Merge branch 'master' of https://github.com/BitWeb/java-core-lib into…

3716d1b

… feature/enable-custom-http-logger-interceptor

feature/enable-custom-http-logger-interceptor

dba7857

feature/enable-custom-http-logger-interceptor

87e3511

Jorich requested changes Oct 31, 2024

View reviewed changes

ennoeller and others added 7 commits October 31, 2024 11:08

feature/enable-custom-http-logger-interceptor

5c38189

Bring back logging level for easier use, default remains same as in 3.x

f4420b7

Remove some non-standard mappers Add no-op interceptor for when logging has been turned off All mappers are extendable and mapper beans can be overridden

Add and improve tests

3b82419

Always upload test artifacts

d357824

Change mapper names to snake_case

2fdc22f

Add tests

Add tests

86f1a4f

Removed old comment

cf3fd18

Jorich previously approved these changes Nov 5, 2024

View reviewed changes

Merge remote-tracking branch 'origin/feature/enable-custom-http-logge…

ef895e0

…r-interceptor' into feature/enable-custom-http-logger-interceptor

Jorich dismissed their stale review via ef895e0 November 5, 2024 09:56

ennoeller marked this pull request as ready for review November 5, 2024 10:19

rammrain approved these changes Nov 5, 2024

View reviewed changes

Jorich approved these changes Nov 5, 2024

View reviewed changes

ennoeller merged commit 29fb286 into master Nov 5, 2024

ennoeller deleted the feature/enable-custom-http-logger-interceptor branch November 5, 2024 10:29


		sb.append(getRequestDescription(request, connection));

		if (loggingLevel.getLevel() >= 2) {

feature/enable-custom-http-logger-interceptor #23

feature/enable-custom-http-logger-interceptor #23

Uh oh!

Conversation

ennoeller commented Aug 30, 2024

Uh oh!

rammrain commented Sep 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rammrain commented Oct 10, 2024

Uh oh!

Jorich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 5, 2024

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants