AGENT-336 Add ability to log to stdout/stderr #415
Conversation
Codecov Report
@@ Coverage Diff @@
## master #415 +/- ##
==========================================
+ Coverage 76.44% 76.51% +0.07%
==========================================
Files 104 104
Lines 26734 26823 +89
==========================================
+ Hits 20436 20524 +88
- Misses 6298 6299 +1
Continue to review full report at Codecov.
|
scalyr_agent/scalyr_logging.py
Outdated
| stdout_handler = self.__stdout_handler | ||
| stderr_handler = self.__stderr_handler | ||
|
|
||
| if force_stdout and stdout_handler: |
There was a problem hiding this comment.
Rather than adding and removing this handler for each emit, we should just re-use the __thread_local__ pattern here. We should have __thread_local__.last_force_stdout and __thread_local__.last_force_stderr that get set here. Then, use that in makeRecord to set attributes force_stdout and force_stderr on the returned record. Finally, add a filter to the stdout and stderr handlers such that they are only used if the record they are asked to handle has force_stdout or force_stderr set to True.
There was a problem hiding this comment.
That would cover any possible race conditions too (I don't think there are any but better safe than sorry), sounds good to me.
|
It looks like if force_stdout/stderr variable is set, we still log to the files (in addition to stdout / stderr), right? Would it make sense to log only to stdout / stder in such scenario (perhaps as an option, because I know this will affect agent.log and agent_debug.log files which won't be shipped to Scalyr in that scenario). On a related note, I think we should enable logging to stdout / stderr by default when running in foreground mode ( |
| @@ -371,6 +371,20 @@ def test_force_valid_metric_or_field_name(self): | |||
| fixed_name2 = scalyr_logging.AgentLogger.force_valid_metric_or_field_name(name2) | |||
| self.assertEqual(fixed_name2, name2) | |||
|
|
|||
| def test_output_to_file_with_stdout(self): | |||
| self.__logger.info("Hello world", force_stdout=True) | |||
There was a problem hiding this comment.
It would be good to assert that the message is also output to stdout (in addition to the file).
For that, we will likely need to patch sys.stdout and sys.stderr.
The intent was to log to stdout/stderr in addition to the usual log paths yes. I don't know of a case where we wouldn't want to send the logs up to Scalyr so I'm not sure how useful a stdout only option would be, maybe @czerwingithub has some insight? Regarding |
Yeah, I'm fine with also logging to the log files in addition to stdout/stderr to not break agent.log + agent_debug.log upload functionality.
I'm fine with a separate flag for k8s / docker deployment. This would offer us more flexibility in the future as well - instead of us trying to programatically detect if we are running inside a container / on Kubernetes we can just rely on a presence of that flag. |
scalyr_agent/scalyr_logging.py
Outdated
| @return: True if the record should be logged by this handler. | ||
| @rtype: bool | ||
| """ | ||
| return hasattr(record, "force_stdout") and record.force_stdout |
There was a problem hiding this comment.
Minor style thing, could also just use getattr aka return getattr(record, "force_stdout", False).
There was a problem hiding this comment.
The hasattr approach matches how its used in the other filters, would it be worth changing all of the uses do you think?
| stdout_bkp = sys.stdout.write | ||
| sys.stdout.write = types.MethodType(self.fake_std_write, sys.stdout) | ||
| try: | ||
| self.__logger.info("Hello world", force_stdout=True) |
There was a problem hiding this comment.
It probably also wouldn't hurt to add a test case for negative / default scenario when force arguments are False.
|
It's a small change.. let's do it for the other places. It would be good
to get a common understanding of good styling and making sure the places we
use it outnumber the places we don't.
…On Tue, Mar 3, 2020 at 1:35 PM yanscalyr ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In scalyr_agent/scalyr_logging.py
<#415 (comment)>:
> + """
+
+ def __init__(self):
+ """Initializes the filter.
+ """
+
+ def filter(self, record):
+ """Performs the filtering.
+
+ @param record: The record to filter.
+ @type record: logging.LogRecord
+
+ @return: True if the record should be logged by this handler.
+ @rtype: bool
+ """
+ return hasattr(record, "force_stdout") and record.force_stdout
The hasattr approach matches how its used in the other filters, would it
be worth changing all of the uses do you think?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#415?email_source=notifications&email_token=AATSXC5GZKT3L656F7ROQKTRFVZZBA5CNFSM4KXM6H6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCX2HFYY#discussion_r387307271>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AATSXC4P6OZGDNRXDPCIID3RFVZZBANCNFSM4KXM6H6A>
.
|
New optional parameters
force_stdoutandforce_stderr.force_stdoutwon't do anything if we are already set to log tostdout.Currently I wrote this by having the loggers add and remove relevant handlers during logging, they are being attached to the actual logger that is being called, and not the one the message eventually propagates to so I don't think there should be any threading issues related to this.