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

Log console default to logging all notebook output? #7386

Closed
jasongrout opened this issue Oct 17, 2019 · 4 comments · Fixed by #7406
Closed

Log console default to logging all notebook output? #7386

jasongrout opened this issue Oct 17, 2019 · 4 comments · Fixed by #7406
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:Design and UX
Milestone

Comments

@jasongrout
Copy link
Contributor

Below is a simple plugin that will log all notebook output to a log console. I think it's too noisy to turn on by default, and the log console should default to just displaying explicit messages that you won't see in a notebook (like the ipywidgets unhandled messages). However, @blink1073 thinks that the log console appears to be broken without something like this.

So should we (a) turn something like this on by default, or (b) provide it as an option, but not turned on by default?

import {
  JupyterFrontEnd,
  JupyterFrontEndPlugin
} from '@jupyterlab/application';
import { ILoggerRegistry } from '@jupyterlab/logconsole';
import { INotebookTracker, NotebookPanel } from '@jupyterlab/notebook';
import { KernelMessage } from '@jupyterlab/services';
import { nbformat } from '@jupyterlab/coreutils';

/**
 * The Log Console extension.
 */
export const logNotebookOutput: JupyterFrontEndPlugin<void> = {
  activate: activateNBOutput,
  id: 'logconsole:nboutput',
  requires: [ILoggerRegistry, INotebookTracker],
  autoStart: true
};

function activateNBOutput(
  app: JupyterFrontEnd,
  loggerRegistry: ILoggerRegistry,
  nbtracker: INotebookTracker
) {
  function registerNB(nb: NotebookPanel) {
    nb.context.session.iopubMessage.connect(
      (_, msg: KernelMessage.IIOPubMessage) => {
        if (
          KernelMessage.isDisplayDataMsg(msg) ||
          KernelMessage.isStreamMsg(msg) ||
          KernelMessage.isErrorMsg(msg) ||
          KernelMessage.isExecuteResultMsg(msg)
        ) {
          const logger = loggerRegistry.getLogger(nb.context.path);
          logger.rendermime = nb.content.rendermime;
          const data: nbformat.IOutput = {
            ...msg.content,
            output_type: msg.header.msg_type
          };
          logger.log({ type: 'output', data });
        }
      }
    );
  }
  nbtracker.forEach(nb => registerNB(nb));
  nbtracker.widgetAdded.connect((_, nb) => registerNB(nb));
}
@jasongrout jasongrout added this to the 1.2 milestone Oct 17, 2019
jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Oct 22, 2019
Notebook output logging made it easier to test the logging console extension. I think it is too much noise for production, though, unless it is behind a option and turned off by default. See jupyterlab#7386
@jasongrout
Copy link
Contributor Author

CC @blink1073 - who seemed like he had some thoughts on this.

@blink1073
Copy link
Contributor

I'd love to have a toggle between "All Messages" and "Unhandled Messages" to have the choice. Not sure about the verbiage of "Unhandled" tho. I'll bring it up for discussion today at the meeting.

@jasongrout
Copy link
Contributor Author

jasongrout commented Oct 23, 2019

From the dev meeting today (with the log levels tweaked to match python, for example):

We'll implement log severity levels: critical, error, warning, info, debug. This severity level is stored at the log model level, and determines what messages are accepted into the log (not just the messages displayed). The default log severity is warning (as it is in python, for example). Normal notebook output will be logged to the info level. Unhandled output can be displayed at the warning level (or perhaps the error level if the output is an error message).

We only store messages that meet the current log level (as done in python or java, for example). We have a max queue size for messages, and we don't want the log to be overwhelmed with messages that are uninteresting and push out messages that are interesting. The mental model here is that a user is controlling the actual logging mechanism, not just the view of the log.

Other alternatives we discussed to having one queue and only storing messages that meet the current log level include:

  • maintain a separate queue for each log level. This can get confusing to display. For example, what if you have one error message and then 100 info messages, with a log limit of 10 messages. In the interleaved display, you see the one error message, then just the last 10 info messages, and it looks like you don't have the first 90 info messages (i.e., the interleaved output seems to go back to the error message, but it's missing the info messages that rotated out of the queue). You can communicate to the user about the different queue styles by displaying some marker to show where each queue level starts, but have to figure out a clean way to do that that is not confusing. You can also have a separate display per log level, but then it's hard to construct the interleaved timeline in your head if you want a holistic view of the messages.
  • maintain a queue message Time To Live, which perhaps tracks with user expectations more cleanly than number of messages. Perhaps even a different TTL for each log level. Again, it is more complicated to communicate to a user about different queue lengths for different log levels.

@jasongrout
Copy link
Contributor Author

I've started implementing this in #7406.

@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Nov 25, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:Design and UX
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants