Skip to content

Improve container logging - #4368

Open
reneleonhardt wants to merge 3 commits into
testcontainers:mainfrom
reneleonhardt:improve-logging
Open

Improve container logging#4368
reneleonhardt wants to merge 3 commits into
testcontainers:mainfrom
reneleonhardt:improve-logging

Conversation

@reneleonhardt

@reneleonhardt reneleonhardt commented Aug 12, 2021

Copy link
Copy Markdown
  • BaseConsumer manages the OutputTypes which should be consumed.
  • New abstract LogConsumer manages the logger instance and allows disabling at runtime.
  • New JavaUtilLogConsumer as alternative to Slf4jLogConsumer.

* BaseConsumer manages the OutputTypes which should be consumed.
* New abstract LogConsumer manages the logger instance and allows disabling at runtime.
* New JavaUtilLogConsumer as alternative to Slf4jLogConsumer.
* Container.hasLogConsumers() and clearLogConsumers() to avoid and remove unwanted loggers.
* GenericContainer.logConsumers is now a Set to prevent duplicate loggers.
@reneleonhardt reneleonhardt changed the title Improve logging Improve container logging Aug 12, 2021
@bsideup

bsideup commented Aug 30, 2021

Copy link
Copy Markdown
Member

Hi @reneleonhardt,

There are a lot of unrelated changes (code style, [list.size()] instead of [0]) - could you please revert them and focus this PR on one change?

You can always follow up with things like hasLogConsumers or Set<...> logConsumers`, just not in the same PR, to make it easier to reason about it.

Thanks!

@reneleonhardt

Copy link
Copy Markdown
Author

There are a lot of unrelated changes (code style, [list.size()] instead of [0]) - could you please revert them and focus this PR on one change?

Just to be sure, do you mean I should remove all fixes for code warnings and make a separate PR for all of them?

You can always follow up with things like hasLogConsumers or Set<...> logConsumers`, just not in the same PR, to make it easier to reason about it.

And then make another PR for things like hasLogConsumers, could you specify a list what you think shouldn't be part of this PR?
From my point of view it was part of the solution to the problems I experienced, that's why I included those changes.

@bsideup

bsideup commented Aug 30, 2021

Copy link
Copy Markdown
Member

@reneleonhardt

Just to be sure, do you mean I should remove all fixes for code warnings and make a separate PR for all of them?

Yes, let's not do them as part of this PR, as otherwise they make it much bigger (and harder to review/accept)

From my point of view it was part of the solution to the problems I experienced, that's why I included those changes.

We may do them in this PR, but then we won't be able to merge it as quickly, as we need to discuss them first and can't accept as is, so I would recommend making the smallest possible change, and follow up with another PR for these.

Thanks!

@reneleonhardt

Copy link
Copy Markdown
Author

Ok, I'll try to disect everything.

@reneleonhardt

Copy link
Copy Markdown
Author

Do you know why Execution failed for task ':testcontainers:japicmp'?
The logs download didn't contain the mentioned build/reports/japi.html.

@bsideup

bsideup commented Aug 31, 2021

Copy link
Copy Markdown
Member

@reneleonhardt it looks like you introduced a binary incompatible change. You can run the same task locally and check the report - it will tell you where.

@reneleonhardt

Copy link
Copy Markdown
Author

@reneleonhardt it looks like you introduced a binary incompatible change. You can run the same task locally and check the report - it will tell you where.

Strange tool, it's doesn't support variadic functions (Java 5), to avoid this "failure" I had to duplicate the constructor now...

@bsideup

bsideup commented Aug 31, 2021

Copy link
Copy Markdown
Member

@reneleonhardt this has nothing to do with what tool supports, that's how Java and its binary compatibility works :)

@reneleonhardt

Copy link
Copy Markdown
Author

Is something missing or can this be merged?

@eddumelendez
eddumelendez requested a review from a team November 19, 2024 22:04
@eddumelendez
eddumelendez requested a review from a team as a code owner August 19, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants