testcontainers-java
testcontainers-java copied to clipboard
testcontainers
Add `docker.` prefix to container logger names, to enable log level to be set across all loggers
Sorry for the bump, any chance of reviewing this? I have a feeling this may help deal with #637 where it would be nice to set the log level for all dockers at once instead of matching against the 🐳...
@vcvitaly please could you merge from master, or preferably give maintainers permission to edit?
We've had some CI problems, so hopefully updating from master should fix things.
Overall I think this looks fine and long overdue (sorry for the immense time to review). Let's just see the tests pass once rebased onto master and I think we should be good to go!
I think it would be fair to say this fixes #637, although not in the way originally proposed.
@rnorth I just can't find that checkbox to allow edits, I don't have it for some reason. I rebased onto the master.
I’m afraid I’ll have to defer this to 1.15.1, so that we can review this together. We’re expecting to publish that release soon after 1.15.0, so it shouldn’t be too much delay.
@rnorth Anything blocking this from being merged now, given that 1.15.1 is released? Would be incredibly nice to make the container logging be more easily controlled from e.g. logback-test.xml config.
@bsideup I think you had some ideas about how this could be done without a change to how it looks for normal use cases?
Ping @bsideup, would be great to get your feedback on this. (Not myself advocating a particular solution, only eager to see #637 resolved one way or the other)
any progress on this? it is a bit more difficult to promote this library when logging is difficult to configure.
(personally, I would prefer an org.testcontainers. prefix, but anything other than root will solve my problems.)
Humble ping :slightly_smiling_face:
org.testcontainers. is definitely better.
Sorry, everyone, I am not sure why we let this go stale for some time now. We will look into it so that we can decide at least whether we will tackle this in a timely manner, or not at all.
Sorry, everyone, I am not sure why we let this go stale for some time now. We will look into it so that we can decide at least whether we will tackle this in a timely manner, or not at all.
Sounds good @kiview, any updates on this yet? :relaxed:
@vcvitaly - I added some feedback in https://github.com/testcontainers/testcontainers-java/pull/3018/files#r1006459726. If you're still with us, it would be nice to get these final details sorted out before this hopefully gets merged.
If not, I am willing to take ownership of this change to make it be mergable. We really, really need to get this shipped anytime soon. It's a major pain related to e.g. CI logging right now.
@vcvitaly - I added some feedback in https://github.com/testcontainers/testcontainers-java/pull/3018/files#r1006459726. If you're still with us, it would be nice to get these final details sorted out before this hopefully gets merged.
If not, I am willing to take ownership of this change to make it be mergable. We really, really need to get this shipped anytime soon. It's a major pain related to e.g. CI logging right now.
@perlun sorry, I almost forgot about this PR) let me review this within two days
@kiview I renamed the logger as suggested by the community, please provide your feedback.
Ping @kiview, is this mergable now or would it require any further adjustments?
@eddumelendez - you seem to have been merging stuff recently. If you could help push things forward on this, it would be very greatly appreciated. :bow: :pray:
Anyone who talks to the maintainers recently, please ping them about this. I seem to be unable to get in touch with them despite repeated attempts.
Hi, sorry for the delay on this one. We just started discussing about this internally. So, we will be sharing news hopefully soon.
Thanks for your patience.
@eddumelendez quite some time has passed since the PR was opened so here is a short review of the reasons why I opened it:
- The initial problem was that since the container logs start with the whale emoji or docker prefix, they go under the root INFO hierarchy, and you cannot see debug logs that might be useful if there are issues. I experienced that myself when I was working on another PR to testcontainers and couldn't find an issue for a while because it was printed as debug and effectively not shown. What I did - I just removed the whale emoji from the factory and added the 'docker' prefix with the DEBUG level to the logback-test.xml because I thought that missing debug info is a mistake. I now understand that it was probably a conscious decision.
- Since then multiple community members have made complaints that it's hard to configure logging with the whale emoji and that a non-Unicode prefix should be used. I then changed the prefix to org.testcontainers.docker as suggested by @perlun
If something else is required from my side that might speed up pr review, I'd be happy to provide any additional info.
#6781 has been created in order to tackle this. Thanks for your contribution @vcvitaly