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

Support '--info' to pipe info logs to stdout #261

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chia7712
Copy link

@chia7712 chia7712 commented Jan 5, 2021

fix #260

@ghost
Copy link

ghost commented Jan 5, 2021

It looks like @chia7712 hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

Copy link
Contributor

@ewencp ewencp left a comment

Choose a reason for hiding this comment

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

I actually like the direction of the patch generally to shift away from debug parameters to more general log_level parameters, though test.py changes in particular would need compatibility tests since I'm pretty sure the change of parameter names would be breaking (e.g. a test against kafka's existing tests; but I haven't checked the exact function calls). Even more generally, rather than continuing to add to the public interface w/ more command line args that are specific to log levels, having a way to feed in a more general logging config would probably be better long term.

That said, we should separate the issue from the solution. The bug report has to do just with timeouts, which can also be solved in other ways. In this case it seems lack of regular output is the issue. Outputting more stuff might help, but does it ultimately fix the problem or just hack around it temporarily? For example, if parallelism=1 and a test takes > 10min, I'm not sure this helps. Additionally, outputting test INFO logs to stdout could have a significant impact on log size for a test run.

In contrast, it seems the need here is simply for something (possibly with a newline and ensuring flushing, which is a common requirement) to be written to stdout periodically to indicate progress. Another way of accomplishing this would be periodic status updates, even if they just repeat the same info (e.g. Running test 12 of 52...). It's been awhile since I looked at that code, but that might be doable in just a few lines of runner.py if timeouts were applied to certain calls, specifically the ones waiting for the test runners to complete (the recv call I believe).

Have we looked into that option? Can we just ensure TestRunner emits something periodically that will keep CI and other systems happy knowing it is doing something?

@chia7712
Copy link
Author

chia7712 commented Jan 5, 2021

@ewencp Thanks for your quick response!

For this timeout issue, it seems to me the control of outputting something to make CI happy should be up to ducktape users, because they are able to set the suitable/meaningful output. It is nice that TestRunner emits something periodically for Kafka but it could be noisy to other users who don't require those periodical outputs.

Personally, adding a "configurable" option to enable/disable much/less messages is more flexible and it can be transparent to "non-kafka" users. WDYT?

I actually like the direction of the patch generally to shift away from debug parameters to more general log_level parameters, though test.py changes in particular would need compatibility tests since I'm pretty sure the change of parameter names would be breaking

oh, I didn't notice related function calls. Will check them later.

@stan-is-hate stan-is-hate requested a review from a team January 21, 2021 01:05
@stan-is-hate
Copy link
Member

I think both --info and periodic printing of status updates are useful in their own ways. When running tests locally via ducker-ak, for example, it would be useful to see all info output.
Periodic printing can also be made configurable (turn it on/off via a separate cmd line flag).

So if I understand the discussion above correctly, there are two open quesions:

  • changing parameter names can break stuff - need to check if it is so or not
  • whether to use --debug/--info flags, having a way to feed in a more general logging config which I think is a good idea for ducktape 1.0 release, but not necessarily required short term. Unless we simply introduce an additional --log-level flag instead of --info.

@cla-assistant
Copy link

cla-assistant bot commented Aug 16, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

Support '--info' to pipe info logs to stdout
3 participants