-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
log timestamp when --debug is presented (#1773) #2266
Conversation
A sample log:
|
3e5377c
to
e0ae22e
Compare
samcli/cli/context.py
Outdated
@@ -40,6 +42,10 @@ def __init__(self): | |||
def debug(self): | |||
return self._debug | |||
|
|||
@property | |||
def output_timestamp(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just configure the logger to have the timestamp when debugging is enabled? Do we really need a new options for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related comment from @awsjeffg
I think the idea is, maybe, for non-debug mode, users will want still to see the timestamp. So having a dedicate flag gives such flexibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfuss You're not wrong, I am just being careful as we've run into other problems where changing stdout behaviour causes customers problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@awsjeffg Yes, we have run into problems with other tools expecting certain output but that has been against our 'info' logs not 'debug'. Additionally, we should be able to freely add data without worrying. I see a stronger case for removing output. I am not saying we shouldn't be cautious around changing output but as a product we need to have the flexibility to update and change output, otherwise we are always stuck with whatever we ship with. I am a strong believer in relying on stdout or stderr output is an anti-pattern and should be treated as behavior not an interface. I am only aware of the AWS Toolkit depending on our output, can't we just ask them if this would break them? My understanding is it shouldn't.
Long ago, we had the default logger for all of SAM CLI's logs. This was overly noisy and we ended up removing it. I personally do not see a difference between adding --time-stamp-output
and using --debug
. They are ways to configure the logging and output. We should be aware of the additional overhead on our customer when adding new options for things. Why should we have an option versus an environment variable? If you want this still to be intentionally, wouldn't something like allowing --debug --debug
to allow customers to control/add additional output (similar to -vv
which is common for command line tools)?
I can understand wanting to introduce this in a new way somehow (though I don't fully agree), but the main thing I want to poke at is: Why a new command line option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are valid in my opinion. Another thought against ONLY including timestamp in DEBUG mode: including timestamp into --debug
which is used for configuring logging level is implicit in my opinion.
Ideally I would prefer to always include timestamps regardless what logging level users chooses to configure. But as @awsjeffg pointed out, it will change stdout behaviour. I understand changing stdout is not something we like to do very often, but --output-timetamp
does feel like a temporary solution.
Considering users only use --debug
mode for investigating (are they?), I think it is fairly safe to change stdout behavior there assuming debug stdout won't be used for postprcessing. What do you think?
8ffbffb
to
d6f7645
Compare
d6f7645
to
bece598
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple questions but overall this looks good.
samcli/cli/context.py
Outdated
for logger_name in [SAM_CLI_LOGGER_NAME, LAMBDA_BULDERS_LOGGER_NAME]: | ||
logger = logging.getLogger(logger_name) | ||
logger.setLevel(logging.DEBUG) | ||
for handler in logger.handlers: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we setting on all handlers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only one handlers I think. In order to get handler object from logger
, only logger.handlers
is available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(no longer applicable in the latest iteration)
samcli/cli/context.py
Outdated
logging.getLogger("samcli").setLevel(logging.DEBUG) | ||
logging.getLogger("aws_lambda_builders").setLevel(logging.DEBUG) | ||
# Turn on debug logging and display timestamps | ||
for logger_name in [SAM_CLI_LOGGER_NAME, LAMBDA_BULDERS_LOGGER_NAME]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticing this. Why are we setting the logger directly instead of through SamCliLogger.configure_logger
like we do in other places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SamCliLogger.configure_logger
will add a new handler. We should change the existing handlers here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I do think we should pack this into SamCliLogger
though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(done)
bece598
to
c8aaaca
Compare
log_stream_handler = handlers[0] | ||
else: | ||
log_stream_handler = logging.StreamHandler() | ||
logger.addHandler(log_stream_handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not need to addHandler()
in the if part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is already a handler, it indicates this method is called before. We only need to change the existing handler instead of creating a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So then we don't need line 32 and 33 above? Can we update this to only do the StreamHanlder if handler is None?
log entry will look like '2020-09-30 20:51:12,353 | ...'
82caa58
to
7a48acb
Compare
log entry will look like '2020-09-30 20:51:12,353 | ...'
Issue #1773, if available:
Why is this change necessary?
How does it address the issue?
What side effects does this change have?
Did you change a dependency in
requirements/base.txt
?If so, did you run
make update-reproducible-reqs
Checklist:
make pr
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.