Add contentLogLevel task property

[fvaleri]

We should not log record's payload by default, as it may contain sensitive information. Anyway it is useful to have a configuration option to enable/disable it for debugging purpose.

Proposing new boolean config named camel.source.logPayload.

[davsclaus]

It appears the existing logging is at DEBUG logging level. This new option dont mention this fact so I would assume end users think its logged at INFO logging or always.

Also it may not only be message body that has sensitive data. Headers can contain autorization tokens, usernames etc. So its probably better to have this option for all of it or nothing.

[oscerd]

This must be applied on all the entity in the exchange not body only

[fvaleri]

It appears the existing logging is at DEBUG logging level. This new option dont mention this fact so I would assume end users think its logged at INFO logging or always.

Yes now is at DEBUG level, which is often set when debugging issues. The proposed option logs at INFO level only if the user explicitly set camel.source.logPayload=true, so it's somewhat independent from log level (unless you set it to OFF).

Also it may not only be message body that has sensitive data. Headers can contain autorization tokens, usernames etc. So its probably better to have this option for all of it or nothing.

It's actually including all of this, maybe the option name is not clear enough. I'm open to suggestions for better wording.

[davsclaus]

You can also have the option about the logging level, we do this in Camel, then you can set it to DEBUG, INFO, OFF etc

[fvaleri]

You can also have the option about the logging level, we do this in Camel, then you can set it to DEBUG, INFO, OFF etc

Good idea. Will do that.

[oscerd]

For the Thread.sleep I'm already opening a PR using Awaitility

[davsclaus]

btw for logging you can use CamelLogger that can do the INFO, DEBUG, OFF etc
https://github.com/apache/camel/blob/master/core/camel-api/src/main/java/org/apache/camel/spi/CamelLogger.java

[fvaleri]

For the Thread.sleep I'm already opening a PR using Awaitility

Thanks

[fvaleri]

Added the log option at the record level. Build is failing because of that flaky test that @oscerd is fixing with another PR.

If we want to use the logging level instead of the boolean parameter, then I would need to create an utility class with a switch-case method to not hard-wire knowledge of what logging system is behind the slf4j facade. In that case we could set it to OFF by default.

[valdar]

@fvaleri can you please rebase this PR on master and push -f on the PR so a Github Action run is triggered afresh and we can see if the flakey tests are fixed?

[fvaleri]

@valdar done, but I still see random build failures:

[ERROR] org.apache.camel.kafkaconnector.CamelSourceTaskTest.testUrlPrecedenceOnComponentProperty  Time elapsed: 0.026 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <2> but was: <1>
	at org.apache.camel.kafkaconnector.CamelSourceTaskTest.testUrlPrecedenceOnComponentProperty(CamelSourceTaskTest.java:230)```

[valdar]

Yep, I will look into it, prepare to more rebases :D

[fvaleri]

Yeah no problem, I'm using the following simple script to test and in addition to testUrlPrecedenceOnComponentProperty there is also testSourcePolling that remains flaky. We must get rid of all those sleeps, I will also have a look later.

#!/bin/sh
for (( i = 1; ; i++ ))
do
  echo "Attempt $i"
  mvn test -pl core -am
  exitcode=$?
  if [ $exitcode -ne 0 ]
  then
    echo "Error at attempt $i"
    exit
  fi
done

[valdar]

I fixed testUrlPrecedenceOnComponentProperty since the purpose of that test wasn't to count messages in a given time frame. Regarding testSourcePolling it is a little bit more complicated since the purpose of the test is to ensure that ~2 messages are received in the given unit of time... we can be a little bit more permissive there, by increasing the number of messages and account for a +/-1 error... I am open to suggestions though.

[fvaleri]

Sent my proposal on a dedicated PR, hope it's useful.

[valdar]

@fvaleri can you rebase this on master (again 😞 ) so we (finally 🥳 ) get it merged?

[fvaleri]

If we want to use the logging level instead of the boolean parameter, then I would need to create an utility class with a switch-case method to not hard-wire knowledge of what logging system is behind the slf4j facade. In that case we could set it to OFF by default.

Sure let me do it. I'm quoting myself here with a different approach, but I would need to explore it. What do you think?

[valdar]

@fvaleri if you want to look into use the log level instead of a boolean you are welcome to do so; before writing your own utility class though, take a look at what @davsclaus suggested: https://github.com/apache/camel/blob/master/core/camel-api/src/main/java/org/apache/camel/spi/CamelLogger.java it might have all the pieces you need.

[fvaleri]

@valdar I think it's a better approach but I need to find some time to play with it. Bear with me, I will update this PR.

[valdar]

In the end have you decided to not use CamelLogger? I am not implying is wrong, I am just asking.

[valdar]

LGTM

[valdar]

Thanks for the contribution @fvaleri !