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

chore: improve master logging #2295

Merged
merged 1 commit into from
Apr 29, 2021

Conversation

neilconway
Copy link
Contributor

@neilconway neilconway commented Apr 28, 2021

Two main changes:

  • Reduce gRPC log level from INFO to WARN. gRPC INFO log messages are
    almost universally about cryptic implementation details, such as

    pickfirstBalancer: HandleSubConnStateChange: 0xc0006a2010, {READY }

  • Squelch a log message ("initializing endpoints for agents") that does
    not seem useful.

Along the way, fix various typos and improve log message consistency.

Description

Test Plan

Commentary (optional)

Checklist

  • User-facing API changes need the "User-facing API Change" label.
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • Licenses should be included for new code which was copied and/or modified from any external code.

@cla-bot cla-bot bot added the cla-signed label Apr 28, 2021
@neilconway neilconway requested a review from stoksc April 28, 2021 21:17
@neilconway
Copy link
Contributor Author

@stoksc Not sure offhand if it is okay to create multiple logrus loggers...

Copy link
Contributor

@stoksc stoksc left a comment

Choose a reason for hiding this comment

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

lgtm

@stoksc
Copy link
Contributor

stoksc commented Apr 28, 2021

from an example in the logrus docs (this page https://pkg.go.dev/github.com/sirupsen/logrus#readme-example):

// Create a new instance of the logger. You can have any number of instances.

I just assumed, based on the thinking logrus.New() would be a weird function to have otherwise.

@stoksc
Copy link
Contributor

stoksc commented Apr 28, 2021

Huh wait, maybe that's multiple instances of the same logger.. I guess I'll read better. wait i think i read fine.

@neilconway neilconway force-pushed the logging-fixes branch 2 times, most recently from bbb4da1 to 3bc7a8e Compare April 28, 2021 22:28
@@ -26,22 +26,25 @@ const jsonPretty = "application/json+pretty"

// NewGRPCServer creates a Determined gRPC service.
func NewGRPCServer(db *db.PgDB, srv proto.DeterminedServer) *grpc.Server {
logger := logrus.NewEntry(logrus.StandardLogger())
logger := logrus.New()
Copy link
Contributor

Choose a reason for hiding this comment

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

I spent some time reading the logrus code - afaict it seems totally fine to have two logrus loggers but I'm always happy to be surprised..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that is my reading as well -- assuming the underlying io.Writer is safe to write to concurrently, which I think should be the case here with os.Stderr.

Two main changes:

  * Reduce gRPC log level from INFO to WARN. gRPC INFO log messages are
    almost universally about cryptic implementation details, such as

      pickfirstBalancer: HandleSubConnStateChange: 0xc0006a2010, {READY <nil>}

  * Squelch a log message ("initializing endpoints for agents") that does
    not seem useful.

Along the way, fix various typos and improve log message consistency.
@neilconway neilconway merged commit b07a086 into determined-ai:master Apr 29, 2021
@neilconway neilconway deleted the logging-fixes branch April 29, 2021 13:43
@dannysauer dannysauer added this to the 0.15.4 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants