cockroachdb / errors

Go error library with error portability over the network
Apache License 2.0
2.07k stars 66 forks source link

Best way to add slice or map of key-value pairs to an error? #79

Closed StevenACoffman closed 2 years ago

StevenACoffman commented 3 years ago

Hi. We use structured logging, and we often have multiple contextual fields that we wish to output, and when we create or wrap an error, we would like to be able to add those fields.

For instance, if I am making a GraphQL query, and the arguments to the query are an "id" and a "userName", and that GraphQL query fails, I would like to make an error with those two fields and their values, so that I can later format a structured log message.

How would I do best do that? errors.WithField("id", id).WithField("userName", userName) does not appear to be a thing?

knz commented 3 years ago

For now our recommended best practice is to

Does this help?

knz commented 3 years ago

I'm going to close this issue due to inactivity. If you have further questions or comments, please let us know.

StevenACoffman commented 2 years ago

I looked at doing this again when I re-encountered your excellent blog post.

We prefer to use the context.Context purely for cancellation purposes, but want to provide lots of key-value fields for errors.

We have a rule "log or return error, but not both", and we log structured log messages with type Fields map[string]interface{}, but there are times when in refactoring, we change our minds and want to convert a log to an error.

knz commented 2 years ago

Have you thought about creating a new custom error type, with the payload you desire?

StevenACoffman commented 2 years ago

Oh, that's an excellent idea! Our typical use case is we start with a sentinel error of a particular type (corresponding to HTTP status codes):

NotFoundKind errorKind = "not found"

Then we wrap it with whatever would be useful for logging purposes.

errors.Wrap(errors.NotFoundKind(),
            "districtName", districtName,
            "districtID", existingDistrict.GetID(),
        )

However, while our bespoke Wrap may be convenient, it doesn't have the depth of careful attention that cockroachdb/errors does in other regards.

knz commented 2 years ago

there's a subpackage telemetrykeys already in the library that you could perhaps use for inspiration

StevenACoffman commented 2 years ago

I was going off of exthttp, but so it looks like I need a protobuf file... which I'm not very familiar with. Something like this?:

syntax = "proto3";
import "google/protobuf/any.proto";
package cockroach.errors.extfields;
option go_package = "extfields";

message EncodedFields {
  map<string, google.protobuf.Any> fields = 1;
}
knz commented 2 years ago

I suppose you could do this, but do you know for sure you can protobuf-encode your values?

generally,b efore you chose an implementation you need to ask yourself:

  1. do you need the key-value pairs to be portable over the network?

    if no here, then you don't need to create an encode/decode pair

  2. does your tech stack already define a standardized way to port payloads over the network?

    if no here, that's going to create a complex conversation within your project. It's not clear that protobuf is always the best choice. Then regardless of tech, you'll need to introduce some new processes and testing best practices to check all this.

  3. if yes to both, then yes what you propose above is a good idea
StevenACoffman commented 2 years ago

Ok, skipping the proto stuff for now sounds like a good idea. Not exactly sure how to redact unsafe keys when printing a map[string]interface{} using the tooling:

func (w *withFields) SafeFormatError(p errors.Printer) (next error) {
    if p.Detail() {
        if p.Detail() && w.fields != nil {
            p.Printf("fields: [")
            redactableTagsIterate(w.fields, func(i int, r redact.RedactableString) {
                if i > 0 {
                    p.Printf(",")
                }
                p.Print(r)
            })
            p.Printf("]")
        }
    }

    return w.cause
}
func redactableFieldsIterate(fields Fields, fn func(i int, s redact.RedactableString)) {
    var empty redact.SafeString
    i := 0
    for k, v := range fields {
        eq := empty
        var val interface{} = empty
        if v != nil {
            if len(k) > 1 {
                eq = ":"
            }
            val = v
        }
        res := redact.Sprintf("%s%s%v", redact.Safe(k), eq, val)
        fn(i, res)
        i++
    }
}

Seems maybe ok? so I pushed it up as: https://github.com/StevenACoffman/erratum

StevenACoffman commented 2 years ago

BTW, if it seems generally useful I would be happy to make a PR for bringing it here.

StevenACoffman commented 2 years ago

Looks like not. Thanks for your help!

knz commented 2 years ago

oh sorry! I'm not subscribed to notifications here.

Since the Go error system is modular and designed to work with multiple go packages implementing errors side by side, you could implement your own go package separate from this one, to provide the error types that you need?

StevenACoffman commented 2 years ago

@knz No worries. I did that: https://github.com/StevenACoffman/erratum and it works fine! Thanks.