fix #4254: adding debug logging for exec stream messages #4288

shawkins · 2022-07-21T11:32:02Z

Description

Add debug logging for streams that don't have explicit handling. This is mostly just an internal nicety, but can also be useful to users, when they are tweaking their commands and don't need/want explicit handling for stderr for example.

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

Code contributed by me aligns with current project license: Apache 2.0
I Added CHANGELOG entry regarding this change
I have implemented unit tests to cover my changes
I have added/updated the javadocs and other documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

manusa · 2022-07-25T07:27:41Z

...es-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/ExecWebSocketListener.java

        webSocket.request();
      }
    }
  }

-  private static final Logger LOGGER = LoggerFactory.getLogger(ExecWebSocketListener.class);
+  static final Logger LOGGER = LoggerFactory.getLogger(ExecWebSocketListener.class);


Could we keep this private and retrieve the logger in PodOperationContext using the same approach?

Also, are we sure that this is the class/package/point where we want to add the flag to decide if these messages should be logged? Can we document how to enable this logging? Would it make sense to check for debug enabled at the base package?

Could we keep this private and retrieve the logger in PodOperationContext using the same approach?

You mean enable this off of two separate loggers? I didn't like that approach, even if they share a common base.

Also, are we sure that this is the class/package/point where we want to add the flag to decide if these messages should be logged?

Yes, they should be logged in the ExecWebSocketListener, and we have to make an earlier decision to include the query flags.

Can we document how to enable this logging?

Yes, we just have to agree on what it should be first.

Would it make sense to check for debug enabled at the base package?

You could, it's just a bit more code to create / access a couple of additional loggers - call me lazy, but I'd rather reuse something that exists if possible.

You mean enable this off of two separate loggers? I didn't like that approach, even if they share a common base.

I meant doing this from PodOperationContext:

boolean debug = // ExecWebSocketListener.LOGGER.isDebugEnabled(); LoggerFactory.getLogger(ExecWebSocketListener.class);

It's still coupled, but at least we don't access the other class variable that should be used for internal purposes only.

However, I'm going over both #4288 (comment) and #4288 (comment) and as I see it's just that the structure is flawed.

The PodOperationContext is not only used for Exec operations, but for other operations too.

It's usage should be clear from the context :)

For me it's not clear that a public addQueryParameters method is used only to add exec-related parameters. Maybe this method just doesn't belong here.

I guess that at this point it doesn't matter anyway.

manusa · 2022-07-25T07:33:29Z

...etes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/PodOperationContext.java

@@ -164,10 +164,11 @@ public void addQueryParameters(URLBuilder httpUrlBuilder) {
    if (in != null || redirectingIn) {
      httpUrlBuilder.addQueryParameter("stdin", "true");
    }
-    if (output != null) {
+    boolean debug = ExecWebSocketListener.LOGGER.isDebugEnabled();


This reveals an odd coupling between ExecWebSocketListener and this class.

The name of the method suggests a wider usage of these parameters, however, they are only ultimately consumed at the public ExecWatch exec(String... command) method. Maybe we should rename the addQueryParameters method.

This reveals an odd coupling between ExecWebSocketListener and this class.

It's partly an issue with the existing code structure. The entry class is PodOperationsImpl, but it's in a different package from the rest of the logic - presumably because there could eventually be a v2. That's how most of dsl.internal is used.

Maybe we should rename the addQueryParameters method.

It's usage should be clear from the context :) You could also make the case that the classes in dsl.internal should move into core.v1 and have methods be made package private.

shawkins · 2022-08-01T12:55:55Z

@manusa based upon the approval, I'll add doc note about this.

It's worth mentioning that an alternative is to directly associate a debug output stream. However as an output stream we loose the ability to discern individual messages. It would be worth considering at some point directly exposing the lower-level onMessage abstraction. That somewhat relates to #4157 and #4201 as well. The only place we use sendAsync with a Reader or InputStream is from log reading calls where we're directly handling the user back the stream abstraction.