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

Add option to enable logging into stdout using env variable #123

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

see-quick
Copy link
Member

@see-quick see-quick commented Dec 6, 2024

This PR adds an option to enable logging into stdout using the env variable STRIMZI_TEST_CONTAINER_LOGGING_ENABLED.

An example of the log can be found [1].

[1] - https://gist.githubusercontent.com/see-quick/8d88a97b533f8910ec2af135071b6ab7/raw/57e6cc73e8a875de75e0655677fac30bba5b67f8/gistfile1.txt

Signed-off-by: see-quick <maros.orsak159@gmail.com>
@see-quick see-quick added the enhancement New feature or request label Dec 6, 2024
@see-quick see-quick self-assigned this Dec 6, 2024
@see-quick see-quick added this to the 0.110.0 milestone Dec 6, 2024
@see-quick see-quick requested a review from a team December 6, 2024 15:33
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you call it logToConsole in one place and BrokerContainerSlf4jLogging in the other? Would the same name everywhere make sense? Also, having javadocs explaining what it actually does would help. console has many meanings for example.

Also, would some environment variable or system property be better for a switch like that? If you aim this for troubleshooting, then you likely do not want to modify the tests to troubleshooth but for example just set an environment variable maybe?

@Frawless
Copy link
Member

Frawless commented Dec 6, 2024

Also, would some environment variable or system property be better for a switch like that? If you aim this for troubleshooting, then you likely do not want to modify the tests to troubleshooth but for example just set an environment variable maybe?

Wouldn't be better to log it to file instead of to console? Guess it can pollute the log quite quickly in case user will have more than few containers.

@see-quick
Copy link
Member Author

see-quick commented Dec 10, 2024

Also, would some environment variable or system property be better for a switch like that? If you aim this for troubleshooting, then you likely do not want to modify the tests to troubleshooth but for example just set an environment variable maybe?

Wouldn't be better to log it to file instead of to console? Guess it can pollute the log quite quickly in case user will have more than few containers.

So in that case I think it would be best to call getLogs() (if something bad happens) and append it to the logs files inside /target/strimzi-test-container/<date> as we have for instance LogCollector.

Also, would some environment variable or system property be better for a switch like that? If you aim this for troubleshooting, then you likely do not want to modify the tests to troubleshooth but for example just set an environment variable maybe?

Yeah, this is a good point. I think it would be better to use ENV or system property here. The main reason it to just enhance current troubleshooting in CI.

Wouldn't be better to log it to file instead of to console? Guess it can pollute the log quite quickly in case user will have more than few containers.

That said we could have both options:

  1. [1] @Frawless, where a user wants to collect all logs and store it somewhere... (using getLogs())
  2. And ENV to enable console output for container.

[1] - #119

@see-quick
Copy link
Member Author

Any opinions on this @scholzj @Frawless? I can start with just:

  1. Add ENV to enable console output for the container.

That would be the easiest way possible.

@Frawless
Copy link
Member

Any opinions on this @scholzj @Frawless? I can start with just:

  1. Add ENV to enable console output for the container.

That would be the easiest way possible.

Yes, sounds reasonable to me.

I am thinking about option to force logging without env for certain situations/tests and I don't have strong opinion. I can imagine situations that might be useful, but user can also achieve ti with ENV option that will of course turn on logging for everything.

Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
@see-quick see-quick requested a review from Frawless December 16, 2024 14:52
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should update the PR description and add something to the README.md file?

@see-quick see-quick changed the title Add option to log container into stdout Add option to enable logging into stdout using env variable Dec 16, 2024
Signed-off-by: see-quick <maros.orsak159@gmail.com>
@see-quick
Copy link
Member Author

Maybe you should update the PR description and add something to the README.md file?

Yeah, thanks for it. I have updated it.

README.md Outdated Show resolved Hide resolved
Signed-off-by: see-quick <maros.orsak159@gmail.com>
Signed-off-by: see-quick <maros.orsak159@gmail.com>
@scholzj scholzj merged commit b697d95 into strimzi:main Dec 17, 2024
7 checks passed
see-quick added a commit that referenced this pull request Dec 17, 2024
* Add option to log container into stdout

Signed-off-by: see-quick <maros.orsak159@gmail.com>

* update use only ENV variable

Signed-off-by: see-quick <maros.orsak159@gmail.com>

* remove un-necessary

Signed-off-by: see-quick <maros.orsak159@gmail.com>

* Add a few words into README.md

Signed-off-by: see-quick <maros.orsak159@gmail.com>

* oops

Signed-off-by: see-quick <maros.orsak159@gmail.com>

* update

Signed-off-by: see-quick <maros.orsak159@gmail.com>

---------

Signed-off-by: see-quick <maros.orsak159@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants