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

📜 Logging tweaks and fixes #498

Merged
merged 2 commits into from
Sep 27, 2020
Merged

📜 Logging tweaks and fixes #498

merged 2 commits into from
Sep 27, 2020

Conversation

c42f
Copy link
Contributor

@c42f c42f commented Sep 27, 2020

  • Prevent frontend from crashing with unknown log levels
    (eg ProgressLogging.@-progress)
  • Avoid spamming the user with infinite log relay errors once the remote
    log channel is closed
  • Only relay info-and-above-level logs from third party modules, to
    prevent excessive debug level log spam.

Fixes a few problems I found while testing #496

* Prevent frontend from crashing with unknown log levels
  (eg ProgressLogging.@-progress)
* Avoid spamming the user with infinite log relay errors once the remote
  log channel is closed
* Only relay info-and-above-level logs from third party modules, to
  prevent excessive debug level log spam.
@fonsp
Copy link
Owner

fonsp commented Sep 27, 2020

Thanks! I rushed this feature a little 😬

About relaying Debug level messages: the browser console actually has a log level filter built in!

Everything enabled:

image

Debug disabled (button on top of the console):

image

We'll have to think about how all of this should be documented, but maybe we can leave in the Debug logs and ask users to filter there? On the other hand, the use case here is to enable logging for setups like github codespaces, binder, SSH, etc, and maybe Debug is only useful for package development, which you wouldn't do on those setups?

@fonsp
Copy link
Owner

fonsp commented Sep 27, 2020

Oh I just saw that you relay Debug from inside the notebook, that's smart

by removing the line highlight and just doing cell highlight instead
@@ -6,26 +6,16 @@ const log_functions = {
}

export const handle_log = ({ level, msg, file, line, kwargs }, filename) => {
const f = log_functions[level]
const f = log_functions[level] || console.log
Copy link
Contributor Author

@c42f c42f Sep 27, 2020

Choose a reason for hiding this comment

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

BTW, we could try to infer more closely whether it should be one of debug, info, warn and error should be used for nonstandard log levels (on the Julia side). But I figured that that console log isn't the UI we want anyway, so just having a crude fallback to bare console.log would be simple for now (and prevent the frontend from crashing)

@c42f
Copy link
Contributor Author

c42f commented Sep 27, 2020

Oh I just saw that you relay Debug from inside the notebook, that's smart

I'm not quite sure what you mean by relay?

All I've done is try to infer what the user cares about: they care about debugging their own notebook code, but not debugging some library which they happen to be using. So I suppressed @debug messages from third party Julia modules. This is partly for user convenience, and partly for performance. Performance-wise I don't think we want the backend sending 10_000s of likely-irrelevant debug log messages, even if they're hidden from the user.

@fonsp fonsp merged commit 178f89c into fonsp:master Sep 27, 2020
@fonsp
Copy link
Owner

fonsp commented Sep 27, 2020

Thanks! While I have you here, would you like to call today?

@c42f
Copy link
Contributor Author

c42f commented Sep 28, 2020

Ah I missed that, sorry :)

@fonsp fonsp added the logging About `@info`, `@warn`, etc label Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logging About `@info`, `@warn`, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants