Improvements to logging #8098
Improvements to logging #8098
Conversation
| const pid = output.pid ? ` (pid: ${output.pid})` : ''; | ||
| const msg = `Python Daemon${pid}: ${output.msg}`; | ||
| if (output.level === 'DEBUG' || output.level === 'NOTSET') { | ||
| traceVerbose(msg); | ||
| } else if (output.level === 'INFO') { | ||
| traceInfo(msg); | ||
| traceVerbose(msg); |
There was a problem hiding this comment.
We don't need any Python logging being displayed in our logs.
If we want that, we can turn on verbose logging.
| @@ -89,7 +89,7 @@ export class PythonKernelDaemon extends BasePythonDaemon implements IPythonKerne | |||
| // This is why when we run `execModule` in the Kernel daemon, it finishes (comes back) quickly. | |||
| // However in reality it is running in the background. | |||
| // See `m_exec_module_observable` in `kernel_launcher_daemon.py`. | |||
| traceInfo(`Starting kernel from scratch with options ${JSON.stringify(options)}`); | |||
| traceVerbose(`Starting kernel from scratch with options ${JSON.stringify(options)}`); | |||
There was a problem hiding this comment.
Turning this into verbose, as optoins contains the full env varialbles as well, I don't think this is necessary, we log a tonne of other information thats more useful.
I've never looked at this.
There was a problem hiding this comment.
I think we could just skip it.
| } | ||
| this.subject.next(out); | ||
| }, | ||
| (out) => this.subject.next(out), |
There was a problem hiding this comment.
I added this because I thought when we have stderr output coming from the kernel it might die.
Found that this is totally unnecessary as the output is sent up and we log this again (here all we do is just log a warning with a message possibly died, which is unnecessary.
| @@ -122,7 +122,7 @@ export namespace traceDecorators { | |||
| const DEFAULT_OPTS: TraceOptions = TraceOptions.Arguments | TraceOptions.ReturnValue; | |||
|
|
|||
| export function verbose(message: string, opts: TraceOptions = DEFAULT_OPTS) { | |||
| return createTracingDecorator([globalLogger], { message, opts }); | |||
| return createTracingDecorator([globalLogger], { message, opts, level: LogLevel.Trace }); | |||
There was a problem hiding this comment.
Bug, when somethign was verbose, we'd still log as info.
| @@ -6,7 +6,7 @@ import { format } from 'winston'; | |||
| import { isTestExecution } from '../common/constants'; | |||
| import { getLevel, LogLevel, LogLevelName } from './levels'; | |||
|
|
|||
| const TIMESTAMP = 'YYYY-MM-DD HH:mm:ss'; | |||
There was a problem hiding this comment.
Don't need year, moves content out further to the right unnecessarily
Codecov Report
@@ Coverage Diff @@
## main #8098 +/- ##
=====================================
Coverage 70% 70%
=====================================
Files 367 367
Lines 22701 22733 +32
Branches 3437 3441 +4
=====================================
+ Hits 16091 16124 +33
- Misses 5222 5223 +1
+ Partials 1388 1386 -2
|
| @@ -32,6 +32,9 @@ function normalizeLevel(name: LogLevelName): string { | |||
| return norm; | |||
| } | |||
| } | |||
| if ((name as any) === 'silly') { | |||
There was a problem hiding this comment.
I don't think we should display Silly in console logs.
| @@ -66,7 +66,11 @@ function logResult(loggers: ILogger[], info: LogInfo, traced: TraceInfo, call?: | |||
| const formatted = formatMessages(info, traced, call); | |||
| if (traced.err === undefined) { | |||
| // The call did not fail. | |||
| if (!info.level || info.level > LogLevel.Error) { | |||
| if (info.level && info.level === LogLevel.Error) { | |||
There was a problem hiding this comment.
Bug, verbose & other levels are not logged
Stop displaying user names in logs (I find lots of users strip out the first few parts of the logs as it contains their user names)
~where possible in our logs (minimizing displaying user names in logs)