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 new CLI options #2237

Closed
wants to merge 3 commits into from
Closed

Add new CLI options #2237

wants to merge 3 commits into from

Conversation

ahopkins
Copy link
Member

Implements and closes #2169

As mentioned here, this adds some new flags the the CLI:

  • --mode Can be either debug or prod
  • PROD mode is new, it bumps logging up to WARNING
  • DEBUG mode will deprecate auto-reload
  • --dev is a shortcut to DEBUG mode + auto-reload
  • --fast is a shortcut to max out the available workers

@ahopkins ahopkins requested a review from a team as a code owner September 11, 2021 22:34
@ahopkins ahopkins marked this pull request as draft September 11, 2021 22:34
@ahopkins ahopkins added the needs tests PR that needs additional tests added label Sep 11, 2021
sanic/__main__.py Outdated Show resolved Hide resolved
Comment on lines +104 to +105
"Run with maximum allowable processes.\nThis should be equal "
"to the number of CPU processes available.\n "
Copy link
Member

Choose a reason for hiding this comment

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

This message is hard to understand. Maybe something along the lines of use all available CPUs?

Copy link
Member

Choose a reason for hiding this comment

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

It is worth a note that this does not (until 22.3) disable access logs, or change logging level (not sure if the latter would affect benchmarks). I suppose this is fine if at 22.3 the default mode is steered towards or even merged with the production mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

I threw together a quick little test to check the performance of a simple loop with different logging.

import logging

import timy

LOOPS = 10_000
RANGE = 1_000
LOG = logging.getLogger(__name__)


@timy.timer(loops=LOOPS)
def normal():
    LOG.setLevel(logging.WARNING)
    c = 0
    for i in range(RANGE):
        c += 1


@timy.timer(loops=LOOPS)
def with_logging_warning():
    LOG.setLevel(logging.WARNING)
    c = 0
    for i in range(RANGE):
        c += 1
        LOG.debug("something")


@timy.timer(loops=LOOPS)
def with_logging_debug():
    LOG.setLevel(logging.DEBUG)
    c = 0
    for i in range(RANGE):
        c += 1
        LOG.debug("something")


normal()
with_logging_warning()
with_logging_debug()

The results on my machine:

Timy executed (normal) for 10000 times in 0.338087 seconds
Timy best time was 0.000031 seconds
Timy executed (with_logging_warning) for 10000 times in 2.469493 seconds
Timy best time was 0.000234 seconds
Timy executed (with_logging_debug) for 10000 times in 77.279574 seconds
Timy best time was 0.007269 seconds

Conclusion: changing the logging level has a huge impact on performance. But, even the existence of the not impacted log call is not trivial either. If we are steering towards trying to impress benchmarks, then limiting or removing unused log calls completely should make a difference.

@Tronic
Copy link
Member

Tronic commented Sep 15, 2021

An afterthought, perhaps we should utimately have --debug for dev without reload, and --dev with reload as you suggested, while the default would be production mode so no need for separate switch for that, and still have --fast to use all CPUs. In particular, this would be avoid the parametric --mode arguments altogether, and reduce the total number of modes which I still find confusing.

The drawback is that the default mode would then have to be a genuine production mode, probably no motd or anything extra on the logs, which then could be confusing for some newcomers.

If this is to be implemented, it would indeed be best to change the default mode to what this PR proposed as --prod right now, but perhaps print a warning about the changed functionality until 22.3. Otherwise it seems odd to introduce a new flag --prod that would have no effect after 22.3, or that would be included in --fast. Or in default mode always print a warning suggesting the use of --dev or --debug for development use, despite it actually running in production mode (but just single process).

As discussed before, --fast should be kept as a modifier that also works in combination with --dev. With --debug maybe not, because it pretty much prevents attaching a debugger (or at least issue a warning if that combination is used).

@Tronic
Copy link
Member

Tronic commented Sep 15, 2021

Most daemons print a single-line "motd" on logs when starting, which is actually informative, perhaps that could be kept in all modes to avoid some of the confusion? Print the address and port being listened to and that it is in production mode using N CPUs (processes), but not the full ASCII art logo?

@Tronic
Copy link
Member

Tronic commented Sep 15, 2021

Related but not to this PR: it would be cool if someone redid the logo in xterm256-color or ANSI color and Unicode graphemes. A lot of Unix software is now taking advantage of these, and most terminals seem to support the extended colors and a fairly large selection of Unicode graphemes, even colored emoji and all that now. Although granted, they might display wrong on less while viewing logs via journalctl or something.

Which brings us to one option, only print the motd when a (suitable) TTY is detected, not if the output is redirected (to syslog). Then it would not need to depend on CLI options at all.

if sys.stdout.isatty():
    # You're running in a real terminal
else:
    # You're being piped or redirected

@Tronic
Copy link
Member

Tronic commented Sep 15, 2021

Also one point, since all CPUs nowadays have hyperthreading, my benchmarks show nearly identical performance no matter if you use the number of physical or logical CPUs, although the double number of workers (i.e. logical CPU count) seems to be a tad faster. This gives some headroom for some workers being stuck on I/O and not consuming a CPU, and I think both Intel and AMD can take advantage of instruction reordering via HT to better avoid pipeline stalls, giving a modest increase in total throughput if but HT cores of a physical CPU are fully loaded (contrary to HT when it was first introduced many years ago, when it would actually slow throughput a a bit when fully loaded, although it still improved scheduling latency, which was particularly helpful for interactive use on Windows which at the time had a very bad kernel scheduler that would otherwise cause noticeable stalls on desktop use).

@Tronic
Copy link
Member

Tronic commented Sep 15, 2021

Yet another concern, I would prefer to keep app.run arguments at least to some degree compatible with the CLI. In particular do the CPU count detection there instead within the CLI frontend (might already be addressed, I didn't look at the code).

@ahopkins
Copy link
Member Author

An afterthought, perhaps we should utimately have --debug for dev without reload, and --dev with reload as you suggested, while the default would be production mode so no need for separate switch for that, and still have --fast to use all CPUs. In particular, this would be avoid the parametric --mode arguments altogether, and reduce the total number of modes which I still find confusing.

I am not opposed to this, with the caveat that we slowly introduce the change so no breaks between now and the end of the year.

probably no motd or anything extra on the logs

🤔 MOTD may be okay still. I am thinking specifically of celery which still has a startup message even when you are running in prod. I suppose maybe we can add an arg or config to suppress it if someone really does not want to add it. I honestly am moving more towards the opinion that we should make it more clear what is being run at startup, which can then provide people with an easy copy/paste for support.

Most daemons print a single-line "motd" on logs when starting, which is actually informative, perhaps that could be kept in all modes to avoid some of the confusion?

Okay... so I think we are in agreement here. This is what I was thinking.

Related but not to this PR: it would be cool if someone redid the logo in xterm256-color or ANSI color and Unicode graphemes.

This is a nice point. I have gone back and forth in my head a lot for a while about whether it would or would not be a good idea to add ANSI colors to logging. This might be a good way to do it. Regarding the logo in particular, in what modes do you think it is appropriate to display it? If it is not so big and in the way, maybe we add it to MOTD if in TTY. No logo if logs are being redirected, and if we have a --no-motd option, then obviously no logo there as well.

Yet another concern, I would prefer to keep app.run arguments at least to some degree compatible with the CLI. In particular do the CPU count detection there instead within the CLI frontend (might already be addressed, I didn't look at the code).

I agree in principle. But, if you are doing app.run yourself, then likely you have some specific reason that you want to control some other aspects manually. In this case, the logic that comes with --fast is sort of irrelevant. So, yes it would be confusing if --debug is not the same as app.run(debug=True), but I don't think all of the CLI functionality needs to be available.

@Tronic
Copy link
Member

Tronic commented Sep 18, 2021

Yet another concern, I would prefer to keep app.run arguments at least to some degree compatible with the CLI. In particular do the CPU count detection there instead within the CLI frontend (might already be addressed, I didn't look at the code).

I agree in principle. But, if you are doing app.run yourself, then likely you have some specific reason that you want to control some other aspects manually. In this case, the logic that comes with --fast is sort of irrelevant. So, yes it would be confusing if --debug is not the same as app.run(debug=True), but I don't think all of the CLI functionality needs to be available.

Could use workers=True for automatic selection, and just have --fast enable that flag for now.

As it turned out, implementing good logic for finding the CPU count is non-trivial, thus it is better if app developers can use the existing logic even with app.run.

@ahopkins
Copy link
Member Author

ahopkins commented Sep 23, 2021

Okay, so I have gone back to the drawing board a little on this:

  1. app.mode is either "prod" or "debug". It follows with app.debug for compat. There is no --mode, and no --prod. You are in prod mode out of the box unless you ask for debug using --debug or debug=True. No change there.
  2. Prod mode stays the same for now so that there is no break, but removes logging, etc in 22.3.
  3. If you are running in a TTY, then there is a message telling you about how you could turn on debug mode.
  4. A more robust MOTD will probably need to wait until the next release, but something like this:
┌───────────────────────┐
│    version 21.9.0     │
├───────────────────────┤
│        host: 0.0.0.0  │
│        port: 8000     │
│        mode: prod     │
│     workers: 12       │
│ auto-reload: disabled │
└───────────────────────┘

@ahopkins
Copy link
Member Author

ahopkins commented Sep 23, 2021

Revised proposal:

Flag Mode Tracebacks Logging Access logs Reload Max workers
--debug DEBUG yes DEBUG yes ^1
PROD no INFO ^2 ^3
--dev DEBUG yes INFO yes yes
--fast yes
  • ^1 --debug to deprecate auto-reloading and remove in 22.3
  • ^2 After 22.3 this moves to WARNING
  • ^3 After 22.3: no

When isatty(), we can add a line that says something like:

Sanic is running in PRODUCTION mode. Consider using '--debug' or --dev' while actively developing your application.

@Tronic
Copy link
Member

Tronic commented Oct 25, 2021

Maybe change --dev to INFO log level too? There is a lot of spam on debug level, mainly because of signal handlers that are not used (or maybe remove that message, possibly making the DEBUG log level suitable for general development).

And in contrast to the current code on this PR, I guess the plan was to remove the --mode argument and only keep the short ones --dev, --debug and --fast, while defaulting to production mode which is neither of those flags? The revised proposal seems great!

@ahopkins ahopkins mentioned this pull request Oct 27, 2021
6 tasks
@ahopkins
Copy link
Member Author

Maybe change --dev to INFO log level too?

I like that. The alternative could be a verbosity level on debug mode.

And in contrast to the current code on this PR, I guess the plan was to remove the --mode argument and only keep the short ones --dev, --debug and --fast, while defaulting to production mode which is neither of those flags?

Yes. I have an implementation locally but it still has a lot of junk in it that I need to cleanup before pushing here. TBH, I might scrap this PR altogether and just create a new one.

@ahopkins
Copy link
Member Author

Moved to #2295

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests PR that needs additional tests added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Production and development modes (default workers)
3 participants