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

Add http.Handler to enable runtime changes (#58) #71

Merged
merged 3 commits into from
May 31, 2016

Conversation

pravj
Copy link
Contributor

@pravj pravj commented May 29, 2016

I have already signed the CLA for my previous pull request #53.


The pull request implements a HTTPHandler struct that takes a Logger instance as a field.
It provides a ChangeLogLevel method that accepts a Level argument and returns a http.Handler.

Right now the handler will set the new log level and returns the response after checking the level via Enabled method. The response string is "true" if the level is in effect and "false" otherwise.

  • What are your comments on this response workflow?
  • I feel that a good enhancement would be passing the log level through the request argument, what do you think?

@pravj
Copy link
Contributor Author

pravj commented May 29, 2016

This is a sample code (gist) I have written that demonstrates its working.

It can be viewed as an application which is trying to load its configurations initially.
So the log level is Debug in starting. The application is emitting both Info and Debug messages.

Once the config files are distributed perfectly (simulated by time.Sleep). It makes a request using the provided Handler and the log level is changed to Info. After that, the Debug messages are ignored.

package main

import (
    "os"
    "fmt"
    "time"
    "net/http"
    "github.com/uber-go/zap"
)

func main() {
    writeSyncer := zap.AddSync(os.Stderr)
    logger := zap.NewJSON(
        zap.Debug,
        zap.Output(writeSyncer),
        zap.ErrorOutput(writeSyncer),
    )

    go func() {
        var i int
        for i <= 4 {
            logger.Debug(fmt.Sprintf("Debug - loading config - time left: %v", 5 - i))
            logger.Info(fmt.Sprintf("Info - loading config - time left: %v", 5 - i))

            time.Sleep(time.Millisecond * 1000)
            i = i + 1
        }

        _, err := http.Get("http://localhost:3000/change")
        if err != nil {
            logger.Fatal("Error in GET request")
        }

        // intentionally leaving the check for response data

        i = 1
        for i <= 4 {
            logger.Debug(fmt.Sprintf("Debug - loaded config - since: %v", i))
            logger.Info(fmt.Sprintf("Info - loaded config - since: %v", i))

            time.Sleep(time.Millisecond * 1000)
            i = i + 1
        }

        os.Exit(0)
    }()

    hh := zap.NewHTTPHandler(logger)

    mux := http.NewServeMux()
    mux.Handle("/change", hh.ChangeLogLevel(zap.Info))

    http.ListenAndServe(":3000", mux)
}

@@ -38,7 +38,7 @@ type Logger interface {
// Check the minimum enabled log level.
Level() Level
// Change the level of this logger, as well as all its ancestors and
// descendants. This makes makes it easy to change the log level at runtime
// descendants. This makes it easy to change the log level at runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for catching this 👍

Based on the comments by @prashantv

GET - Returns the current log level as JSON string '{"level":"info"}'
PUT - Updates the log level and returns the effective one as JSON string

Returns 'Method Not Allowed' (405) for any other method.

Returns 'Bad Request' (400) for bad request data or unrecognized level.
@akshayjshah
Copy link
Contributor

Great - thanks for being so flexible, @pravj! There are a few small style nits remaining, but I can fix those up in a follow-up change - I'll tag you in the PR so you can see the changes.

@pravj
Copy link
Contributor Author

pravj commented May 30, 2016

@akshayjshah @prashantv
Thanks for your guidance, I believe I have been disturbing you with my questions.

I have learned so much just by reading the source code of zap, it's awesome.

  • Should we also log events happening in the handler like 'receiving a request', 'bad request' etc.?

@akshayjshah
Copy link
Contributor

Nope, this is great as-is. At least for now, let's not log.

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

Successfully merging this pull request may close these issues.

3 participants