[errors] de-specialize Transient in favor of Tags.

[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'common/retry/transient' package. 
Marking an error as transient is now `err = transient.Tag.Apply(err)`, or 
`errors.Annotate(err).Tag(transient.Tag).Err()` (as part of another batch
of annotations).

API Review in a Nutshell:
  // for simple 'boolean' tags, where presence is the only important piece of
  // information.
  var FatalTag = errors.BoolTag{errors.NewTagKey("this is a fatal error!")}

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated typesafe values
  type Severity int
  type severityTagType struct {Key errors.TagKey}
  func (t severityTagType) With(s Severity) errors.TagValue { return errors.MkTagValue(t.Key, s) }
  func (t severityTagType) In(err error) (s Severity, ok bool) {
    d, ok := errors.TagValueIn(t.Key, err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTagKey("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.In(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=

Review-Url: https://codereview.chromium.org/2951393002
Committed: https://github.com/luci/luci-go/commit/585415ced4abef0b0ceff6c495690df98db83abb

[iannucci, 2017-06-23 23:27:47]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 23:27:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 23:42:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-23 23:42:27]

Dry run: Try jobs failed on following builders:
  Luci-go Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36f01a41f1d1af10)

[iannucci, 2017-06-23 23:57:12]

Description was changed from

==========
[errors] de-specialize Transient in favor of Tags.

BUG=
==========

to

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.NewBoolTag("this is a fatal error!")

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated values (complete with type assertions)
  type Severity int
  var SeverityTag = errors.NewTagger("holds a Severity value", func(v
interface{}) {
    _ = v.(Severity)
  })

  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.ValueIn(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

[iannucci, 2017-06-23 23:57:12]

iannucci@chromium.org changed reviewers:
+ dnj@chromium.org, nodir@chromium.org, vadimsh@chromium.org

[iannucci, 2017-06-23 23:58:09]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-23 23:58:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iannucci, 2017-06-24 00:01:12]

PTAL

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go
File common/errors/tags.go (right):

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:1: // Copyright 2017 The LUCI Authors. All rights
reserved.
This is where the bulk of the new logic lives.

[commit-bot: I haz the power, 2017-06-24 00:11:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-24 00:11:27]

Dry run: Try jobs failed on following builders:
  Luci-go Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36f0360b0a3f9910)

[dnj, 2017-06-24 14:53:55]

Nice work. I like it, but I have a lot of thoughts and a few concerns. It's a
big CL, so I kind of wrote as I reviewed, so apologies in advance if there are
any inconsistencies.

https://codereview.chromium.org/2951393002/diff/80001/appengine/datastorecach...
File appengine/datastorecache/cache.go (right):

https://codereview.chromium.org/2951393002/diff/80001/appengine/datastorecach...
appengine/datastorecache/cache.go:339: return retry.Tag.Apply(err)
I don't like "retry.Tag.Apply" as a way of expressing transience. I think
transience and the will to retry are two different-but-related concepts. I'd
recommend "retry.Transient.Apply", or just doing your transient package idea.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/annotate.go
File common/errors/annotate.go (right):

https://codereview.chromium.org/2951393002/diff/80001/common/errors/annotate....
common/errors/annotate.go:87: FrameInfo stackFrameInfo
Let's make these internal members (lower-case)?

https://codereview.chromium.org/2951393002/diff/80001/common/errors/annotate....
common/errors/annotate.go:206: func (s *stackContext) AddData(data Data) {
nit: Let's lower-case these method names too?

https://codereview.chromium.org/2951393002/diff/80001/common/errors/annotate....
common/errors/annotate.go:327: // `value` is a uniary optional argument, and
must be a simple type (i.e. has
nit: "unary"

https://codereview.chromium.org/2951393002/diff/80001/common/errors/annotate....
common/errors/annotate.go:654: tse.tags = make(map[tagKey]interface{},
len(tags))
Handle all of the cases that "Tag" handles (e.g., t == nil).

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go
File common/errors/tags.go (right):

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:50: BoolTag interface {
I'd:
1) Make the NewBoolTag method BoolTag.
2) Have it return interface{TagKey, TagValue}
3) Delete this interface.

I think the naming is better, and I don't think there is any good reason to
export this type, is there?

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:89: // however preventing this cast makes the whole thing
much more complex.
(Unless you delete the BoolTag interface)

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:105: a := &Annotator{err, stackContext{FrameInfo:
stackFrameInfoForError(1, err)}}
We're dong a full captureStack every time we apply a tag to an error? Should
tags have an associated stack? Applying several tags to an annotated error could
result in a lot of stack captures.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:109: func (t tagKey) With(val interface{}) TagValue {
I don't like the binary "Apply" XOR "With" as an interface. Either split this
into two interfaces (one with value, one without) so we can statically verify
usage, or make no-value versions of these functions just treat the value as
"nil".

I'm extra concerned here b/c oftentimes obscure error generation paths are the
ones that aren't tested too much, and someone could get a random production
panic because they mess up something that could be compile-time checked.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:116: case k < reflect.Array || k == reflect.String:
I think it would be better to just enumerate the "reflect.*" types that *are*
allowed. A user getting a panic right now would have to pull up the "reflect"
godocs and read the enumeration values to understand this.

That or explicitly document allowed types in the "With" function.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:118: panic("Tagger.With(value) only takes simple types for
`value`, not " + k.String())
Why? I get that you're trying to not have this be used as a data channel, but
really if someone wants to do something dumb like that, they're going to in some
way or another. Why is it your job to stop them? :)

Also concerned about loss of static checking. For example, someone has:

func whatever() error {
  v := thing() // Returns int
  result, err := otherThing()
  if err != nil {
    return errors.Annotate(err).Tag(MyTag.With(v))
  }
  return consumeThing(v)
}

Yay, this works! Except the owner of "thing" library changes the return type
from a simple type to a complex type. Code still compiles, but PANIC! This isn't
good, and errors are too primitive to open this door IMO.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:120: if detail.validate != nil {
Similar concerns for validation, as either validation or the type being passed
in could change w/out any warning and make it into production code.

If we really want to inspect this, let's set up a situation where users can
declared typed tags:

type MyTag struct {
  errors.BasicTagger
  Value MyType
}

func (t *MyTag) With(val MyType) TagValue { ... }

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:131: // ValueIn implements TagKey.
Comment not needed, or comment other methods uniformly.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:132: func (t tagKey) ValueIn(err error) (value
interface{}, ok bool) {
Is it OK that this traverses MultiError? Probably, but just wanted to check.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:150: a := &Annotator{err, stackContext{FrameInfo:
stackFrameInfoForError(1, err)}}
(Same, full stack trace seems like a not great thing to do for each "Apply" if
one has already been taken)

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:163: func NewBoolTag(description string) BoolTag {
Note that there's nothing stopping someone from naively doing this within a
handler or loop or something else stupid and causing a memory leak.

I don't really have a good way to adapt this model to accommodate that, but it
seems possible enough that it's concerning to me.

https://codereview.chromium.org/2951393002/diff/80001/grpc/grpcutil/errors.go
File grpc/grpcutil/errors.go (right):

https://codereview.chromium.org/2951393002/diff/80001/grpc/grpcutil/errors.go...
grpc/grpcutil/errors.go:22: ret = retry.Tag.Apply(ret)
This is concerning to me WRT current behavior. I like it, but we should think
hard about its implications.

e.g., previously someone could use "retry.WithTransient" / Transient to tell a
retry loop to retry just "NotFound". Now, it will retry NotFound and other
transient errors.

[iannucci, 2017-06-24 20:15:59]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-24 20:16:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iannucci, 2017-06-24 20:16:10]

PTAnL

https://codereview.chromium.org/2951393002/diff/80001/appengine/datastorecach...
File appengine/datastorecache/cache.go (right):

https://codereview.chromium.org/2951393002/diff/80001/appengine/datastorecach...
appengine/datastorecache/cache.go:339: return retry.Tag.Apply(err)
On 2017/06/24 14:53:54, dnj wrote:
> I don't like "retry.Tag.Apply" as a way of expressing transience. I think
> transience and the will to retry are two different-but-related concepts. I'd
> recommend "retry.Transient.Apply", or just doing your transient package idea.

Done.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/annotate.go
File common/errors/annotate.go (right):

https://codereview.chromium.org/2951393002/diff/80001/common/errors/annotate....
common/errors/annotate.go:87: FrameInfo stackFrameInfo
On 2017/06/24 14:53:54, dnj wrote:
> Let's make these internal members (lower-case)?

Done.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/annotate....
common/errors/annotate.go:206: func (s *stackContext) AddData(data Data) {
On 2017/06/24 14:53:54, dnj wrote:
> nit: Let's lower-case these method names too?

Done.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/annotate....
common/errors/annotate.go:327: // `value` is a uniary optional argument, and
must be a simple type (i.e. has
On 2017/06/24 14:53:54, dnj wrote:
> nit: "unary"

Done.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/annotate....
common/errors/annotate.go:654: tse.tags = make(map[tagKey]interface{},
len(tags))
On 2017/06/24 14:53:54, dnj wrote:
> Handle all of the cases that "Tag" handles (e.g., t == nil).

Done.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go
File common/errors/tags.go (right):

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:50: BoolTag interface {
On 2017/06/24 14:53:55, dnj wrote:
> I'd:
> 1) Make the NewBoolTag method BoolTag.
> 2) Have it return interface{TagKey, TagValue}
> 3) Delete this interface.
> 
> I think the naming is better, and I don't think there is any good reason to
> export this type, is there?

Done.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:89: // however preventing this cast makes the whole thing
much more complex.
On 2017/06/24 14:53:55, dnj wrote:
> (Unless you delete the BoolTag interface)

They could still cast it to TagValue

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:105: a := &Annotator{err, stackContext{FrameInfo:
stackFrameInfoForError(1, err)}}
On 2017/06/24 14:53:54, dnj wrote:
> We're dong a full captureStack every time we apply a tag to an error? Should
> tags have an associated stack? Applying several tags to an annotated error
could
> result in a lot of stack captures.

If you apply it singly, then yes, if you apply many tags there's only a single
stack capture (using errors.Annotate.Tag(...))

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:109: func (t tagKey) With(val interface{}) TagValue {
On 2017/06/24 14:53:55, dnj wrote:
> I don't like the binary "Apply" XOR "With" as an interface. Either split this
> into two interfaces (one with value, one without) so we can statically verify
> usage, or make no-value versions of these functions just treat the value as
> "nil".
> 
> I'm extra concerned here b/c oftentimes obscure error generation paths are the
> ones that aren't tested too much, and someone could get a random production
> panic because they mess up something that could be compile-time checked.

I actually don't understand this comment at all... What do you mean by XOR?

That said, I rewrote all of this to be type safe!! Go didn't make it easy, but I
did it :).

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:116: case k < reflect.Array || k == reflect.String:
On 2017/06/24 14:53:54, dnj wrote:
> I think it would be better to just enumerate the "reflect.*" types that *are*
> allowed. A user getting a panic right now would have to pull up the "reflect"
> godocs and read the enumeration values to understand this.
> 
> That or explicitly document allowed types in the "With" function.

ALL OF THIS CODE IS GONE NOW!

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:118: panic("Tagger.With(value) only takes simple types for
`value`, not " + k.String())
On 2017/06/24 14:53:55, dnj wrote:
> Why? I get that you're trying to not have this be used as a data channel, but
> really if someone wants to do something dumb like that, they're going to in
some
> way or another. Why is it your job to stop them? :)
> 
> Also concerned about loss of static checking. For example, someone has:
> 
> func whatever() error {
>   v := thing() // Returns int
>   result, err := otherThing()
>   if err != nil {
>     return errors.Annotate(err).Tag(MyTag.With(v))
>   }
>   return consumeThing(v)
> }
> 
> Yay, this works! Except the owner of "thing" library changes the return type
> from a simple type to a complex type. Code still compiles, but PANIC! This
isn't
> good, and errors are too primitive to open this door IMO.

I was trying to prevent people from putting mutable data into an error so they
don't:

  value := []byte{...}
  error := ...Tag.With(value).Apply(err)
  // modify value here or elsewhere, probably not by pulling it out of the
error,
  // but because something else has a ref to value

But this is all gone now :)

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:120: if detail.validate != nil {
On 2017/06/24 14:53:55, dnj wrote:
> Similar concerns for validation, as either validation or the type being passed
> in could change w/out any warning and make it into production code.
> 
> If we really want to inspect this, let's set up a situation where users can
> declared typed tags:
> 
> type MyTag struct {
>   errors.BasicTagger
>   Value MyType
> }
> 
> func (t *MyTag) With(val MyType) TagValue { ... }

Done.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:131: // ValueIn implements TagKey.
On 2017/06/24 14:53:55, dnj wrote:
> Comment not needed, or comment other methods uniformly.

Acknowledged.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:132: func (t tagKey) ValueIn(err error) (value
interface{}, ok bool) {
On 2017/06/24 14:53:55, dnj wrote:
> Is it OK that this traverses MultiError? Probably, but just wanted to check.

yes, I have a test for it too

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:150: a := &Annotator{err, stackContext{FrameInfo:
stackFrameInfoForError(1, err)}}
On 2017/06/24 14:53:55, dnj wrote:
> (Same, full stack trace seems like a not great thing to do for each "Apply" if
> one has already been taken)

If you need to apply more than one tag, use the Annotator interface.

https://codereview.chromium.org/2951393002/diff/80001/common/errors/tags.go#n...
common/errors/tags.go:163: func NewBoolTag(description string) BoolTag {
On 2017/06/24 14:53:55, dnj wrote:
> Note that there's nothing stopping someone from naively doing this within a
> handler or loop or something else stupid and causing a memory leak.
> 
> I don't really have a good way to adapt this model to accommodate that, but it
> seems possible enough that it's concerning to me.

That's why I had "New" here. It would also mean that it would be impossible to
check to see if the error had the tag too, so I'm not too worried about it (as
in: they could apply the tag but not read it back).

https://codereview.chromium.org/2951393002/diff/80001/grpc/grpcutil/errors.go
File grpc/grpcutil/errors.go (right):

https://codereview.chromium.org/2951393002/diff/80001/grpc/grpcutil/errors.go...
grpc/grpcutil/errors.go:22: ret = retry.Tag.Apply(ret)
On 2017/06/24 14:53:55, dnj wrote:
> This is concerning to me WRT current behavior. I like it, but we should think
> hard about its implications.
> 
> e.g., previously someone could use "retry.WithTransient" / Transient to tell a
> retry loop to retry just "NotFound". Now, it will retry NotFound and other
> transient errors.

I'll undo this for now then

[iannucci, 2017-06-24 20:22:53]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-24 20:22:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iannucci, 2017-06-24 20:24:55]

Description was changed from

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.NewBoolTag("this is a fatal error!")

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated values (complete with type assertions)
  type Severity int
  var SeverityTag = errors.NewTagger("holds a Severity value", func(v
interface{}) {
    _ = v.(Severity)
  })

  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.ValueIn(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

to

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.NewBoolTag("this is a fatal error!")

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated values (complete with type assertions)
  type Severity int
  type severityTagType struct {errors.TagKey}
  func (s severityTagType) With(s Severity) errors.TagValue { return
s.TagKey.With(s) }
  func (s severityTagType) ValueIn(err error) (s Severity, ok bool) {
    d, ok := s.TagKey.ValueIn(err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = errors.NewTagger("holds a Severity value", func(v
interface{}) {
    _ = v.(Severity)
  })


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.ValueIn(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

[iannucci, 2017-06-24 20:25:23]

Description was changed from

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.NewBoolTag("this is a fatal error!")

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated values (complete with type assertions)
  type Severity int
  type severityTagType struct {errors.TagKey}
  func (s severityTagType) With(s Severity) errors.TagValue { return
s.TagKey.With(s) }
  func (s severityTagType) ValueIn(err error) (s Severity, ok bool) {
    d, ok := s.TagKey.ValueIn(err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = errors.NewTagger("holds a Severity value", func(v
interface{}) {
    _ = v.(Severity)
  })


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.ValueIn(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

to

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.NewBoolTag("this is a fatal error!")

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated values (complete with type assertions)
  type Severity int
  type severityTagType struct {errors.TagKey}
  func (s severityTagType) With(s Severity) errors.TagValue { return
s.TagKey.With(s) }
  func (s severityTagType) ValueIn(err error) (s Severity, ok bool) {
    d, ok := s.TagKey.ValueIn(err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTag("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.ValueIn(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

[iannucci, 2017-06-24 20:26:02]

Description was changed from

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.NewBoolTag("this is a fatal error!")

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated values (complete with type assertions)
  type Severity int
  type severityTagType struct {errors.TagKey}
  func (s severityTagType) With(s Severity) errors.TagValue { return
s.TagKey.With(s) }
  func (s severityTagType) ValueIn(err error) (s Severity, ok bool) {
    d, ok := s.TagKey.ValueIn(err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTag("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.ValueIn(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

to

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.NewBoolTag("this is a fatal error!")

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated values (complete with a typesafe interface)
  type Severity int
  type severityTagType struct {errors.TagKey}
  func (s severityTagType) With(s Severity) errors.TagValue { return
s.TagKey.With(s) }
  func (s severityTagType) ValueIn(err error) (s Severity, ok bool) {
    d, ok := s.TagKey.ValueIn(err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTag("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.ValueIn(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

[iannucci, 2017-06-24 20:26:17]

Description was changed from

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.NewBoolTag("this is a fatal error!")

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated values (complete with a typesafe interface)
  type Severity int
  type severityTagType struct {errors.TagKey}
  func (s severityTagType) With(s Severity) errors.TagValue { return
s.TagKey.With(s) }
  func (s severityTagType) ValueIn(err error) (s Severity, ok bool) {
    d, ok := s.TagKey.ValueIn(err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTag("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.ValueIn(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

to

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.NewBoolTag("this is a fatal error!")

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated typesafe values
  type Severity int
  type severityTagType struct {errors.TagKey}
  func (s severityTagType) With(s Severity) errors.TagValue { return
s.TagKey.With(s) }
  func (s severityTagType) ValueIn(err error) (s Severity, ok bool) {
    d, ok := s.TagKey.ValueIn(err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTag("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.ValueIn(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

[commit-bot: I haz the power, 2017-06-24 20:35:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-24 20:35:26]

Dry run: Try jobs failed on following builders:
  Luci-go Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36f49738b7016a10)

[iannucci, 2017-06-25 19:30:59]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-25 19:31:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-25 19:45:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-25 19:45:19]

Dry run: Try jobs failed on following builders:
  Luci-go Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36f98e2f3325d410)

[dnj, 2017-06-26 15:50:52]

I think the custom struct is the right direction. I thought about this for a
bit, and wrote out a long (apologies in advance) proposal that I think fits
pretty good here. WDYT?

https://codereview.chromium.org/2951393002/diff/140001/common/errors/tags.go
File common/errors/tags.go (right):

https://codereview.chromium.org/2951393002/diff/140001/common/errors/tags.go#...
common/errors/tags.go:20: TagKey int
I still don't like the global lock and counter.
- Massive memory leak if someone uses NewTag in a handler/loop, and no way to
control or prevent this.
- Local variable tags may actually be useful.

Can we make a TagKey a pointer to a struct? It will still be unique, and we
won't need to worry about locking or a global registry. Equality will still
work, too.

type TagKey *tagDetail

...

func NewTag(description string) TagKey { return &tagDetail{description} }

You can't declare methods on TagKey, but the only really important one is
String(); however, string for this TagKey would just show: &{"description"},
which is fine?

IMO we should get rid of the boolean / value tag as standalone constructs; since
tags are inexorably linked to Annotator, let's just store tag values in the
Annotator code base and leave the "tags.go" focusing on tag keys. Annotator
would store the key/value tuple itself, instead of having the tag self-describe
to it. This will also make "tagsToMap" unnecessary, as we can just use
"a.tagMap". Annotator can expose some generic functions, to be used by tag
implementations:

Change Apply to helper methods for your new tag struct layout:

ApplyTag(err error, t TagKey, value interface{}) { // new Annotator layer w/
this tag/value }

Replace With, In, and ValueIn with Annotator methods:

func (a *Annotator) GetTag(t TagKey) (interface{}, bool)
func (a *Annotator) HasTag(t TagKey) bool

Make a (key, value) tuple the only supported structure (i.e., no boolean magic)
within Annotator, and use generators to make this clean (example at bottom):

type TagGenerator interface{
  Generate func() (TagKey, interface{})
}

func (a *Annotator) Tag(tg ...TagGenerator) *Annotator

Finally, add some top-level methods to help handle generic errors and build
helper functions for tag structs:

func GetTag(err error, t TagKey) (interface{}, bool) {
  if aerr, ok := err.(*Annotator) {
    return aerr.GetTag(t)
  }
  return nil, false
}

func HasTag(err error, t TagKey) (
  if aerr, ok := err.(*Annotator) {
    return aerr.HasTag(t)
  }
  return false
}

Define a generic BoolTag, that is a TagGenerator:

type BoolTag struct { TagKey }
func (bt BoolTag) Generate() (TagKey, interface{}) { return bt.TagKey, nil }
func (bt BoolTag) Apply(err error) error { return ApplyTag(err, bt.TagKey, nil)
}
func (bt BoolTag) In(err error) bool { return HasTag(err, bt.TagKey) }

///
Boolean tags are simple!
///

var Foo = errors.BoolTag{ errors.NewTag("FOO!") }

///
Value tags are more manual, since we want typed accessors. However, your new
per-tag
struct refactor makes this relatively clean.
///

type barTag struct { errors.TagKey }
func (bt barTag) Apply(err error, d string) error { return errors.ApplyTag(err,
bt.TagKey, d) }
func (bt barTag) In(err error) bool { return errors.HasTag(err, bt.TagKey) }
func (bt barTag) Value(err error) string {
  if v, ok := errors.GetTag(err, bt.TagKey); ok {
    return v.(string)
  }
  return ""
}
func (bt barTag) With(d string) errors.TagGenerator {
  return func() (errors.TagKey, interface{}) { return bt.TagKey, v }
}

var Bar = barTag{ errors.NewTag("FOO!") }

///
Resulting in:
///

return errors.Annotate(err).Tag(Foo, Bar.With("BAR!")).Err()

[iannucci, 2017-06-27 01:06:27]

The CQ bit was checked by iannucci@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 01:06:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 01:23:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 01:23:10]

Dry run: This issue passed the CQ dry run.

[iannucci, 2017-06-27 01:26:07]

Description was changed from

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.NewBoolTag("this is a fatal error!")

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated typesafe values
  type Severity int
  type severityTagType struct {errors.TagKey}
  func (s severityTagType) With(s Severity) errors.TagValue { return
s.TagKey.With(s) }
  func (s severityTagType) ValueIn(err error) (s Severity, ok bool) {
    d, ok := s.TagKey.ValueIn(err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTag("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.ValueIn(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

to

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.BoolTag{errors.NewTagKey("this is a fatal error!")}

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated typesafe values
  type Severity int
  type severityTagType struct {Key errors.TagKey}
  func (t severityTagType) With(s Severity) errors.TagValue { return
errors.MkTagValue(t.Key, s) }
  func (t severityTagType) In(err error) (s Severity, ok bool) {
    d, ok := errors.TagValueIn(t.Key, err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTagKey("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.In(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

[iannucci, 2017-06-27 01:28:27]

ptal, this is a hybrid approach.

I renamed ValueIn to just `In`. It makes sense to establish the semantics for
this on a per-tag basis. For example, in the BoolTag, there's not a value to
distinguish presence vs. value. In something like a severity tag, it might make
sense to have severity.Tag.In(err) always be defined to some Severity value
(like 0).

Like maybe grpcutil wants to have the default Code be Internal (if the error
wasn't explicitly tagged otherwise).

[iannucci, 2017-06-27 02:12:17]

On 2017/06/27 01:28:27, iannucci wrote:
> ptal, this is a hybrid approach.
> 
> I renamed ValueIn to just `In`. It makes sense to establish the semantics for
> this on a per-tag basis. For example, in the BoolTag, there's not a value to
> distinguish presence vs. value. In something like a severity tag, it might
make
> sense to have severity.Tag.In(err) always be defined to some Severity value
> (like 0).

I just re-read this and realized it was confusing. To clarify:
  * BoolTags now actually hold a bool, and default to `true`
  * If you consider a tag like Severity (in the CL commit message), it might
make sense 
     to have the severity default to some value (like highest/lowest severity),
if that tag is
     absent from the error.

> 
> Like maybe grpcutil wants to have the default Code be Internal (if the error
> wasn't explicitly tagged otherwise).

[dnj, 2017-06-27 03:57:03]

This lgtm. Thanks for iterating!

Just a few comments. None are blockers, more just thoughts.

https://codereview.chromium.org/2951393002/diff/160001/common/errors/tags.go
File common/errors/tags.go (right):

https://codereview.chromium.org/2951393002/diff/160001/common/errors/tags.go#...
common/errors/tags.go:34: TagValueGenerator interface {
There are a lot of tag functions. I'm seeing two classes of users:

1) Casual library users, who don't give a crap about tags here, because they use
the actual tags that are declared (e.g., transient.Tag).
2) People who want to create their own tags.

WDYT about moving all of this into "errors/tags" for (2), to keep the "errors"
API simple for (1)? Can remove the word "Tag" from a lot of these
functions/types :)

The only tricky part is "Apply" referencing Annotator. I wonder if Annotator
could operate on Tags as an interface so that this dependency tree is not
cyclic.

Just a thought.

https://codereview.chromium.org/2951393002/diff/160001/common/errors/tags.go#...
common/errors/tags.go:82: // Off allows you to "remove" this boolean tag from an
error (by setting it to
Not sure if this is a good thing or not. Oh well, we'll see :)

https://codereview.chromium.org/2951393002/diff/160001/grpc/grpcutil/errors.go
File grpc/grpcutil/errors.go (right):

https://codereview.chromium.org/2951393002/diff/160001/grpc/grpcutil/errors.g...
grpc/grpcutil/errors.go:90: type grpcCodeTag struct{ Key errors.TagKey }
QQ: why is "Key" named here (and elsewhere)?

[iannucci, 2017-06-27 18:14:28]

https://codereview.chromium.org/2951393002/diff/160001/common/errors/tags.go
File common/errors/tags.go (right):

https://codereview.chromium.org/2951393002/diff/160001/common/errors/tags.go#...
common/errors/tags.go:34: TagValueGenerator interface {
On 2017/06/27 03:57:03, dnj wrote:
> There are a lot of tag functions. I'm seeing two classes of users:
> 
> 1) Casual library users, who don't give a crap about tags here, because they
use
> the actual tags that are declared (e.g., transient.Tag).
> 2) People who want to create their own tags.
> 
> WDYT about moving all of this into "errors/tags" for (2), to keep the "errors"
> API simple for (1)? Can remove the word "Tag" from a lot of these
> functions/types :)
> 
> The only tricky part is "Apply" referencing Annotator. I wonder if Annotator
> could operate on Tags as an interface so that this dependency tree is not
> cyclic.
> 
> Just a thought.

I considered it; as it is, the errors library deletes a lot of symbols in this
cl (net negative) anyway, so it's a win. Refactoring the tag creation to another
package didn't have an obvious way to do it. I think it's fine to
revisit+iterate on this some more.

https://codereview.chromium.org/2951393002/diff/160001/common/errors/tags.go#...
common/errors/tags.go:82: // Off allows you to "remove" this boolean tag from an
error (by setting it to
On 2017/06/27 03:57:03, dnj wrote:
> Not sure if this is a good thing or not. Oh well, we'll see :)

Yeah, we'll see. I put it in because otherwise it wouldn't be a very good "Bool"
tag. I also considered naming this NilTag, but that was just confusing.

https://codereview.chromium.org/2951393002/diff/160001/grpc/grpcutil/errors.go
File grpc/grpcutil/errors.go (right):

https://codereview.chromium.org/2951393002/diff/160001/grpc/grpcutil/errors.g...
grpc/grpcutil/errors.go:90: type grpcCodeTag struct{ Key errors.TagKey }
On 2017/06/27 03:57:03, dnj wrote:
> QQ: why is "Key" named here (and elsewhere)?

Because TagKey is a pointer type, it can't be embedded. Not that it matters,
because it can't have any methods on it anyway...

[iannucci, 2017-06-27 19:25:55]

Description was changed from

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'retry' package (though we may want
to have a specific 'transient' package, so that `transient.Tag.Apply(err)` is a
thing)

After some brief discussion, it looks like we may want to formalize some error 
classification scheme (maybe using grpc.Code) to use as a primary tag.

API Review in a Nutshell:
  // for simple 'boolean' tags; they are either present or not
  var FatalTag = errors.BoolTag{errors.NewTagKey("this is a fatal error!")}

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated typesafe values
  type Severity int
  type severityTagType struct {Key errors.TagKey}
  func (t severityTagType) With(s Severity) errors.TagValue { return
errors.MkTagValue(t.Key, s) }
  func (t severityTagType) In(err error) (s Severity, ok bool) {
    d, ok := errors.TagValueIn(t.Key, err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTagKey("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.In(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

to

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'common/retry/transient' package. 
Marking an error as transient is now `err = transient.Tag.Apply(err)`, or 
`errors.Annotate(err).Tag(transient.Tag).Err()` (as part of another batch
of annotations).

API Review in a Nutshell:
  // for simple 'boolean' tags, where presence is the only important piece of
  // information.
  var FatalTag = errors.BoolTag{errors.NewTagKey("this is a fatal error!")}

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated typesafe values
  type Severity int
  type severityTagType struct {Key errors.TagKey}
  func (t severityTagType) With(s Severity) errors.TagValue { return
errors.MkTagValue(t.Key, s) }
  func (t severityTagType) In(err error) (s Severity, ok bool) {
    d, ok := errors.TagValueIn(t.Key, err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTagKey("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.In(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

[iannucci, 2017-06-27 20:58:16]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2017-06-27 20:58:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 21:31:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-27 21:31:19]

Try jobs failed on following builders:
  Luci-go Presubmit on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/37042aa87d5eb610)

[iannucci, 2017-06-27 21:42:46]

Yay now our builds fail nondeterministically due to npm stuff :'(

[iannucci, 2017-06-28 17:55:38]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2017-06-28 17:55:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 18:13:15]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1498672538853610,
"parent_rev": "2ae56cd555233728c302e9a67d229fc35aaf007f", "commit_rev":
"585415ced4abef0b0ceff6c495690df98db83abb"}

[commit-bot: I haz the power, 2017-06-28 18:13:19]

Description was changed from

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'common/retry/transient' package. 
Marking an error as transient is now `err = transient.Tag.Apply(err)`, or 
`errors.Annotate(err).Tag(transient.Tag).Err()` (as part of another batch
of annotations).

API Review in a Nutshell:
  // for simple 'boolean' tags, where presence is the only important piece of
  // information.
  var FatalTag = errors.BoolTag{errors.NewTagKey("this is a fatal error!")}

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated typesafe values
  type Severity int
  type severityTagType struct {Key errors.TagKey}
  func (t severityTagType) With(s Severity) errors.TagValue { return
errors.MkTagValue(t.Key, s) }
  func (t severityTagType) In(err error) (s Severity, ok bool) {
    d, ok := errors.TagValueIn(t.Key, err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTagKey("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.In(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=
==========

to

==========
[errors] de-specialize Transient in favor of Tags.

Previously 'transience' was a hard-coded attribute of the errors package. This
change adds 'tags' as a way to put attributes (potentially with some data) onto
an
error as it goes up the stack.

This removes multiple symbols from the errors package (the stack context stuff
is now an implementation detail, tags are better than ExtractData). 

The 'transience' bit has now moved to the 'common/retry/transient' package. 
Marking an error as transient is now `err = transient.Tag.Apply(err)`, or 
`errors.Annotate(err).Tag(transient.Tag).Err()` (as part of another batch
of annotations).

API Review in a Nutshell:
  // for simple 'boolean' tags, where presence is the only important piece of
  // information.
  var FatalTag = errors.BoolTag{errors.NewTagKey("this is a fatal error!")}

  func (...) error {
    return FatalTag.Apply(err)
  }

  func (...) {
    if FatalTag.In(err) {
      os.Exit(-1)
    }
  }

  // tags may also have associated typesafe values
  type Severity int
  type severityTagType struct {Key errors.TagKey}
  func (t severityTagType) With(s Severity) errors.TagValue { return
errors.MkTagValue(t.Key, s) }
  func (t severityTagType) In(err error) (s Severity, ok bool) {
    d, ok := errors.TagValueIn(t.Key, err)
    if ok {
      s = d.(Severity)
    }
    return
  }
  var SeverityTag = severityTagType{errors.NewTagKey("holds a Severity value")}


  func (...) error {
    return SeverityTag.With(Severity(10)).Apply(err)
  }

  func (...) {
    if s, hasSeverity := SeverityTag.In(err); hasSeverity {
      if s.(Severity) > 9000 {
        fmt.Println("it's pretty severe")
      }
    }
  }

BUG=

Review-Url: https://codereview.chromium.org/2951393002
Committed:
https://github.com/luci/luci-go/commit/585415ced4abef0b0ceff6c495690df98db83abb
==========

[commit-bot: I haz the power, 2017-06-28 18:13:21]

Committed patchset #9 (id:160001) as
https://github.com/luci/luci-go/commit/585415ced4abef0b0ceff6c495690df98db83abb