Skip to content

add log verbosity#1380

Merged
jeremyrsmith merged 1 commit intopolynote:masterfrom
skilld-labs:add-log-verbosity
Mar 7, 2023
Merged

add log verbosity#1380
jeremyrsmith merged 1 commit intopolynote:masterfrom
skilld-labs:add-log-verbosity

Conversation

@dhia-gharsallaoui
Copy link
Contributor

@dhia-gharsallaoui dhia-gharsallaoui commented Feb 10, 2023

Hello 👋

This PR add the possibility to configure the logger verbosity using the configuration file.

###############################################################################
# The logger verbosity level: info, warn, error (default: info)
###############################################################################

log:
  verbosity: info

the default value is Info to keep the old default behavior.

In addition, I set the verbosity after logging the configuration, securityWarning, polynote banner and the version. What do you think about that?

this PR is done with the love and the help of @ahuret and @skilld-labs

@omidmogasemi
Copy link
Collaborator

omidmogasemi commented Feb 13, 2023

Hey again! Thanks for taking the time for this PR. I will take a closer look with Jeremy sometime this week, but at first glance this looks great!

the default value is Info to keep the old default behavior.

In addition, I set the verbosity after logging the configuration, securityWarning, polynote banner and the version. What do you think about that?

Both of these are good points that seem right to me!

@jbguerraz
Copy link
Contributor

jbguerraz commented Mar 2, 2023

Hello,
Did you have any chance to spend a time with @jeremyrsmith on this small PR @omidmogasemi ?
Thank you a lot!

@jeremyrsmith
Copy link
Contributor

jeremyrsmith commented Mar 7, 2023 •
edited
Loading

Hi @dhia-gharsallaoui, thanks for the PR!

My only issue with it is the global mutable state of the logger. But, for something like this, I think that's relatively minor. We can refactor it if it becomes an issue.

jeremyrsmith
jeremyrsmith reviewed Mar 7, 2023
View reviewed changes
polynote-kernel/src/main/scala/polynote/kernel/logging/package.scala
blocking: Blocking.Service => new Logging.Service.Default(System.err, blocking)
}

object Verbosity extends Enumeration {
Copy link
Contributor

@jeremyrsmith jeremyrsmith Mar 7, 2023

Choose a reason for hiding this comment

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

Not a big fan of Enumeration but I guess it gives you the ordering for free in this case.

@jeremyrsmith jeremyrsmith merged commit 508494c into polynote:master Mar 7, 2023
@dhia-gharsallaoui
Copy link
Contributor Author

dhia-gharsallaoui commented Mar 7, 2023

Thank you for the review @jeremyrsmith !
I want to mention that we still have the spark logs which come as REMOTE are not managed by this PR.

@jeremyrsmith
Copy link
Contributor

jeremyrsmith commented Mar 7, 2023

Thanks @dhia-gharsallaoui for the PR. I think it's OK not to deal with remote logs for now. A nice thing to do would be to configure files for log output, and have separate files for each kernel's (like remote logs) vs the polynote server's logs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeremyrsmith jeremyrsmith jeremyrsmith left review comments

@omidmogasemi omidmogasemi Awaiting requested review from omidmogasemi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dhia-gharsallaoui @omidmogasemi @jbguerraz @jeremyrsmith