go-kit / kit Goto Github PK

When a package or component takes a log.Logger, it should just use it, and not do any work to wrap it in a context. Contextualization of loggers should happen in the caller.

Log15 has a very nice API for structured logging which is compatible with varargs style logging (see here: https://github.com/inconshreveable/log15#faq).

Could this be implemented?

Not really a coding issue. But it would be great to have a repository / directory of all the go-kit 3rd party middlewares available :-)

• http://godoc.org/github.com/sony/gobreaker
• http://godoc.org/github.com/streadway/handy/breaker

It doesn't seem right for the loadbalancer package to have domain-specific Publisher implementations inside it.

BadRequestError has error as embedded type. Since error starts with small cap, it is an implicit unexported field of BadRequestError. That means only code within transport/http package can access the raw error value. This leave no room for ErrorEncoder implementation to unpack error messages of custom error types.

Suggest either to:

1. To add a BadRequestError.Raw{} method that returns the raw error inside.
2. Change BadRequestError from type struct{error} to type struct{ Raw: error }.

Since golang/go#11904, the context package has a nice wrapper around http.Client that is context aware. Would there be any interest in a PR that uses that package internally to wrap the http.Client so cancel()s will propagate to the request?

A type consulPublisher struct which implements Publisher via a (set of) Consul instance(s).

If you don't explicitly create and pass in a Logger when creating a service, the service will panic when it tries to log an error.

Fixes:

1. fail to create new service without logger
2. default a logger if none created
3. Profit?

Appdash, from SourceGraph.

External clients should not be able to manipulate the mutex,
which is an implementation detail.

With the logger Log of

    logger = log.NewContext(log.NewLogfmtLogger(os.Stderr)).
        With("ts", log.DefaultTimestampUTC,
        "caller", log.Caller(6))
    Log = levels.New(logger)

I get

If I change the log.Valuer to func() string, then everything works.
I think maybe a fmt.Stringer would be enough (or anything what gopkg.in/logfmt.v0 accepts).

A nice example might be an API gateway, which hosts routes like /api/:service/:method, and demuxes them to individual (defined) services.

logger := log.NewLogfmtLogger(os.Stderr)
logger = log.NewContext(logger).With("caller", log.DefaultCaller)
lvlLogger := levels.New(logger)
lvlLogger.Info("hi", "there")

In the output you will get: caller=levels.go:72

This is fixed by using log.Caller(4) instead of log.DefaultCaller but makes passing and extending the logger harder than it should be.

https://github.com/hprose/hprose-go

Hprose is a High Performance Remote Object Service Engine.

It is a modern, lightweight, cross-language, cross-platform, object-oriented, high performance, remote dynamic communication middleware. It is not only easy to use, but powerful. You just need a little time to learn, then you can use it to easily construct cross language cross platform distributed application system.

Hprose supports many programming languages, for example:

• AAuto Quicker
• ActionScript
• ASP
• C++
• Dart
• Delphi/Free Pascal
• .NET(C#, Visual Basic...)
• Golang
• Java
• JavaScript
• Node.js
• Objective-C
• Perl
• PHP
• Python
• Ruby
• ...

Through Hprose, You can conveniently and efficiently intercommunicate between those programming languages.

json.Encode doesn't do that and people are more likely to implement fmt.Stringer then json.Marshaler.

issues to consider:

• should it favor json.Marshaler before fmt.Stringer

json_logger.go

func merge(dst map[string]interface{}, k, v interface{}) {
    var key string
    switch x := k.(type) {
    case string:
        key = x
    case fmt.Stringer:
        key = safeString(x)
    default:
        key = fmt.Sprint(x)
    }
    if x, ok := v.(error); ok {
        v = safeError(x)
}

    switch x := v.(type) {
    case string:
        v = x
    case fmt.Stringer:
        v = safeString(x)
    default:
        v = fmt.Sprint(x)
    }
    if x, ok := v.(error); ok {
        v = safeError(x)
    }
dst[key] = v
}

There are a lot of useful debug HTTP handlers to add for processes. Stuff like pprof, job management endpoints (quitquitquit, abortabortabort, health are common), process-level metrics, etc.

It would be nice to have gokit automatically spin up a debug port for listening with a bunch of defined (and possibly extendable) debug HTTP handlers.

This issue would benefit from #1, since by default we could have it listen on 127.0.0.1:0, but be configurable to be something else.

When using the publisher/factory/load balancer workflow, there is no mechanism currently provided for a publisher to trigger the cleanup of an endpoint which is no longer provided.

1. If the Scribe collector is down, we should drop the batch on the floor, rather than growing it indefinitely in memory.
2. We should provide support for sampling. Need to investigate how that is commonly done.
3. Remove package global logger in favor of logger parameter to relevant constructors (h/t @rogpeppe)

I've noticed that using NewJSONLogger that caller is always an emtpy {} when logging:
​
logger = log.NewJSONLogger(os.Stderr)
logger = log.NewContext(logger).With("caller", log.DefaultCaller)
​logger.Log()
​
Results in the following log output:
​
{"caller":{}}

In stringsvc1 example, when you send a normal string to /uppercase you get {"v":"HELLO","err":null}, but when you send an empty string "", you won't see the error in response {"v":"","err":{}}.

If this is an expected behavior then what is the recommended way to get the ErrEmpty as part of the response?

Would be useful if that is explained in the example documentation.

Thanks!
Gunjan

As an example of how to use it.

The Problem

logger.Log("caller", log.DefaultCaller, "foo", "bar")

does not work the same as

logger = log.With(logger, "caller", log.DefaultCaller)
...
logger.Log("foo, "bar")

Detailed Explantion

The current log package supports dynamic values in log contexts by passing a Valuer to With. A Valuer is a function that withLogger calls and replaces with the return value when its Log method is invoked. For example:

logger := log.With(baseLogger, "caller", log.DefaultCaller)
...
logger = log.With(logger, "req", reqID) // pretend reqID == 10
...
logger.Log("foo", "bar")

Which would result in the withLogger essentially performing:

baseLogger.Log("caller", stack.Caller(3), "req", 10, "foo", "bar)

Unfortunately the similarity in how we pass key/value arguments to With and Log leads to the expectation that the following should work.

baseLogger.Log("caller", log.DefaultLogger, "req", 10, "foo", "bar)

Expanding DefaultLogger we see that the result is a function value, not a call site value.

baseLogger.Log("caller", func() interface{} { return stack.Caller(3) }, "req", 10, "foo", "bar)

The Challenge

There is a subtle bit of magic in the current implementation. No matter how many layers of context are built up DefaultCaller is always three stack frames below the call to withLogger.Log. This holds because With and withLogger take care to flatten the stored log context and copy the base logger into a new withLogger rather than creating a new layer each time.

Unfortunately not all calls to Log go through withLogger, which is why value binding is currently only supported on arguments to With.

The challenge is to support logging a stack.Call referring to the top level call to Logger.Log regardless of whether it is stored in a log context created by log.With or provided directly to Logger.Log. The solution must be convenient for application code to use and not change the Logger interface.

Counter Argument

Value binding is not strictly needed when calling Log as the application could simply provide the bound value directly. Why wrap it in a late binding mechanism only to immediately perform the binding? Could better package documentation be the solution?

Since clients of Publisher always request-then-use an endpoint, we might be able to simplify the idea of a Publisher, removing the concept of a stream of endpoints, and instead providing something more like

type Something interface {
    Get() Endpoint
}

If an error is returned from a requestDecoder passed to http.NewServer, it is wrapped by the unexported badRequestError type before it is passed to the errorEncoder.

Unfortunately, this means the type of the original error, thrown by the requestDecoder, is lost.

If using a customer errorEncoder, you will not be able to switch on the error type to determine if it's one of your own errors, you must look at the error text itself.

• thriftsvc
• netrpcsvc
• grpcsvc
• jsonhttpsvc

type MyService interface {
    Square(int) (int, error)
    Cube(int) (int, error)
}

• https://github.com/inconshreveable/log15
• https://github.com/Sirupsen/logrus

@ChrisHines @tsenart

Rationale: Gauges are measured in various accuracies and are difficult to rescale later once you already have collected data. The performance impact seems negligible, since we only Set and Add Gauges. Doubling the memory consumption per Gauge seems worth the result.

When logging, every value is checked if it implements error interface. When positive, Error method is called. This may cause panic when passed value is nil pointer to structure that use it's internal state to produce Error method optput.

logfmt

The server package should probably have a convention for threading contexts (blog.golang.org/context) through from service handlers all the way down to client requests. In the absence of thread-local-storage (which I believe many Zipkin clients end up using), we need a way to pass through request context from service to service.

(I feel compelled to point out that https://github.com/jtolds/gls is technically a possibility, but it's horrible and I would vote against it)

Filing an issue so we consider unified configuration of some kind.

Motivating example (though definitely not the only case): let's say a gokit process does want to do Dapper/Zipkin style tracing. The process will need to know where to forward the tracing information to, which won't be known at compile time.

It would be nice if all of the gokit packages were configured in the same or similar way, so that as they are used modularly, configuration of subpackages happens automatically, with little programmer assistance.

There should be an easy way to do e.g.

logger.Logf("server started on %s", listenAddress) // default/predefined key
logger.Printf("msgkey", "%d queued, %d finished", queued, finished) // explicit key

oh hey, would be nice to have Sourcegraph enabled on this repo, would help with code reviews. For instance I was reading #6 and the large diff would be easier to read with type annotation =P

this is a canned message they provide:

I want to use Sourcegraph code search and code review with gokit. A project maintainer needs to enable it to set up a webhook so the code is up-to-date there.

Could you please enable gokit on @sourcegraph by going to https://sourcegraph.com/github.com/peterbourgon/gokit and clicking on Settings? (It should only take 15 seconds.)

Thank you!

Given a service interface definition

type FooService interface {
    Bar(ctx context.Context, i int, s string) (string, error)
}

It should be possible to automatically generate a stub service implementation, request and response types, endpoint factories, an endpoints struct, and transport bindings.

type stubFooService struct{}

func (s stubFooService) Bar(ctx context.Context, i int, s string) (string, error) {
    return "", errors.New("not implemented")
}

type BarRequest struct {
    I int
    S string
}

type BarResponse struct {
    S   string
    Err error
}

type makeBarEndpoint(s FooService) endpoint.Endpoint {
    return func(ctx context.Context, request interface{}) (interface{}, error) {
        req := request.(BarRequest)
        s, err := s.Bar(ctx, req.I, req.S)
        return BarResponse{S: s, Err: err}, nil
    }
}

type Endpoints struct {
    Bar endpoint.Endpoint
}

// Each transport binding should be opt-in with a flag to kitgen.
// Here's a basic sketch of what HTTP may look like.
// n.b. comments should encourage users to edit the generated code.

func NewHTTPHandler(endpoints Endpoints) http.Handler {
    m := http.NewServeMux()
    m.Handle("/bar", httptransport.NewServer(
        endpoints.Bar,
        DecodeBarRequest,
        EncodeBarResponse,
    )
    return m        
}

func DecodeBarRequest(_ context.Context, r *http.Request) (interface{}, error) {
    var req BarRequest
    err := json.NewDecoder(r.Body).Decode(&req)
    return req, err
}

func EncodeBarResponse(_ context.Context, w http.ResponseWriter, response interface{}) error {
    w.Header().Set("Content-Type", "application/json; charset=utf-8")
    return json.NewEncoder(w).Encode(response)
}

The CLI should have a UX like

$ kitgen -h
USAGE
  kitgen path/to/service.go [flags]

FLAGS
  -repo-layout default     default, flat, ...
  -allow-no-context false  allow service methods to omit context parameter

The default repo layout should look like addsvc in the go-microservices repo; the flat repo layout should put all files and types in the same package. Other layout options could be considered.

• Manual service discovery (list of endpoints)
• DNS-based service discovery (single IP and SRV)
• Consul
• etcd

/cc @dahernan can you drop your use-case in here? It would be very helpful!

• https://godoc.org/github.com/tsenart/tb
• ?

—

I propose that gokit/log.Logger should specify that implementations must be safe for concurrent use. Logger clients should not need to explicitly coordinate with each other.

Rationale

Logging is an application concern and should be controlled by applications. Each application should decide how log events are filtered and routed. Widely reusable (open source) libraries shouldn't do any logging, preferring to return errors or provide some other mechanism for event notification and leave it to the application to hook that into its choice for logging if desired. Organizations may enforce a logging standard for all of their internal packages. In that case internal libraries can reasonably assume a standard approach to logging and produce log events in accordance, but the application should still control filtering and routing.

Recommended Best Practice

Each source of log events should provide a mechanism for accepting a Logger implementation from the main application. The scope of the accepted Logger determines the granularity of control over filtering and routing available. Some possibilities:

• Function arguments
• Context value or constructor argument
• Package scoped function or variable

Packages that do not create log events do not need any of these. Thus, log.DefaultLogger is not needed as package log is not a source of log events.

Of the above choices, a package scoped logger is attractive. Ignoring concurrency issues for the time being, each package that creates log events could include a declaration such as:

var Log log.Logger = log.NewDiscardLogger()

Now the application can control logging for each package by changing its Log variable. If an application wanted to identify the source package for each log event it could do so by supplying a different contextual logger to each package. For example:

packageA.Log = log.With(logger, "module", "packageA")

httpclient.NewClient could be renamed to httpclient.NewEndpoint, to reduce confusion. Also, audit other packages that generate or consume endpoints for similar improvements to names.

Context

I would like the ability to do something like this:

var Log log.Logger

func HandleTask(taskID int) error {
    logger := log.With(Log, "task", taskID)
    logger.Info("event", "start")
    ...
    logger.Debug("event", "query", "query", query)
    if err := doQuery(query); err != nil {
        logger.Error("event", "query", "err", err)
        return err
    }
    ...
    logger.Info("event", "done")
}

The important aspect of my example is the re-use of logging context across multiple severity levels.

The problem

As it stands now, package log encourages us to implement log levels via multiple Loggers each with a different level context created by a call to log.With. Unfortunately this approach makes it cumbersome to create additional contextual loggers on top of the leveled loggers. For example:

var lvls = log.NewLevels()

func HandleTask(taskID int) error {
    dlog := log.With(lvls.Debug, "task", taskID)
    ilog := log.With(lvls.Info, "task", taskID)
    elog := log.With(lvls.Error, "task", taskID)

    ilog.Log("event", "start")
    ...
    dlog.Log("event", "query", "query", query)
    if err := doQuery(query); err != nil {
        elog.Log("event", "query", "err", err)
        return err
    }
    ...
    ilog.Log("event", "done")
}

In addition to being cumbersome, each call to log.With costs allocations.

Challenge

Can we find a good way to avoid the need for multiple calls to log.With when doing leveled logging?

Are you interested in supporting websockets, I need to have a webosocket connection between my microservices and I would love to be able to use gokit with all of its tooling. If yes, I can work on it, and submit a PR.

A type etcdPublisher struct which implements Publisher via typical etcd service discovery pattern(s).

Hi,

case that is covered already by log package is passing log.Log instance deeper into dependency tree. But what about bubbling error up. I would like to propose new feature called LoggableError.

package log

type LoggableError struct {
    Err error
    Fields []interface{} // maybe a map?
}

func (le *LoggableError) Error() string {
    return le.Err.Error()
}

Using this struct we can wrap any error with extra fields and bubble it up so it can be logged in a single place with all gathered fields eg.:

func extendLoggerWithError(logger Log, err error) logger {
        switch e := err.(type) {
    case *pq.Error:
                return log.With(logger, "code", e.Code, "details", e.Detail, "hint", e.Hint, "table", e.Table, "constraint", e.Constraint)
    case *log.LoggableError:
        return extendLoggerWithError(log.With(logger, e.Fields...), e.Err)
    default:
        return logger
    }

    return logger
}

There're lots of go discussion/channels in gophers' slack community
Should we create a gokit there?

kit/log package is using panic in several places, that are executed conditionally. While the code that is causing it (odd number of key-value pair elements) is logically invalid, panic is not something expected by external package, especially logging.
Odd number of elements means that there is one key with no value. My proposal is to pair such key with empty value. While it's not perfect, it does not break the code and can be detected later while looking at logs or by code analyzers.
Similar logic is provided for example by fmt package - using XXXf with invalid amount of parameters does not panic, but rather produce result with warning. Wrong parameters are also detected by go vet.

• https://github.com/go-kit/kit/blob/master/log/log.go#L47
• https://github.com/go-kit/kit/blob/master/log/log.go#L61
• https://github.com/go-kit/kit/blob/master/log/log.go#L82

• Investigate using davecheney/godoc2md to produce READMEs for each package, including only the package summaries, not the full API. This may require adapting the tool.
• Finish the rationale sections.
• Add proper example functions to illustrate usage, rather than excerpts in the READMEs.