[errors] Greatly simplify common/errors package.

[errors] Greatly simplify common/errors package.

Now that we have tags with values, the original concept behind the 'data'
stuff in errors no longer applies. This removes the Reason().D() system and puts
the format directly into errors.Annotate. So:
  errors.Annotate(err).Reason("foo: %(datum)s").D("datum", "value").Err()

Becomes:
  errors.Annotate(err, "foo %s", "value").Err()

This also removes a bunch of rarely-or-never-used symbols from common/errors by
making them internal (particularly w.r.t. rendering an error). If we ever develop
a need for them, it would be easy to bring them back.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://github.com/luci/luci-go/commit/2224066a0cc0b0fa150c733a614116c87807637e

[iannucci, 2017-06-28 02:58:15]

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

[commit-bot: I haz the power, 2017-06-28 02:58:18]

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-28 03:02:27]

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

[commit-bot: I haz the power, 2017-06-28 03:02:28]

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

[iannucci, 2017-06-28 03:15:19]

On 2017/06/28 02:58:14, iannucci wrote:

Oh, rietveld, y u so dum?

[iannucci, 2017-06-28 19:19:54]

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

[commit-bot: I haz the power, 2017-06-28 19:20:05]

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-28 19:24:28]

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

[commit-bot: I haz the power, 2017-06-28 19:24:29]

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

[iannucci, 2017-06-29 00:40:03]

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

[commit-bot: I haz the power, 2017-06-29 00:40:11]

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-29 00:44:18]

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

[commit-bot: I haz the power, 2017-06-29 00:44:20]

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

[iannucci, 2017-06-29 00:51:04]

Description was changed from

==========
[errors] Make Reason a normal formatter function.

Now that we have tags with values, the original concept behind the 'data'
stuff in errors no longer applies. The Reason and InternalReason associate
human debugging information (internal or external) with the error, and the
Tags associate machine-actionable metadata with the error.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=
==========

to

==========
[errors] Greatly simplify common/errors package.

Now that we have tags with values, the original concept behind the 'data'
stuff in errors no longer applies. This removes the Reason().D() system and puts
the format directly into errors.Annotate. So:
  errors.Annotate(err).Reason("foo: %(datum)s").D("datum", "value").Err()

Becomes:
  errors.Annotate(err, "foo %s", "value").Err()

This also removes a bunch of rarely-or-never-used symbols from common/errors by 
making them internal (particularly w.r.t. rendering an error). If we ever
develop
a need for them, it would be easy to bring them back.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=
==========

[iannucci, 2017-06-29 01:03:45]

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

[commit-bot: I haz the power, 2017-06-29 01:03:50]

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-29 01:08:39]

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

[commit-bot: I haz the power, 2017-06-29 01:08:40]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/370a31d87a36dd10)
  Luci-go Win Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/370a31d8135d1810)

[Vadim Sh., 2017-06-29 02:08:31]

lgtm with minor comments

main one is about non-determinist of tags map traversal

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

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:96: //  I am an internal reson formatted with key1:
value
typo: 'reason'

also, is this comment still up-to-date?

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:119: for key, val := range s.tags {
we probably want to sort tags for consistent output. Getting visually different
error messages for same error is frustrating.

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:178: a.ctx.internalReason = fmt.Sprintf(reason,
args...)
woot!

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:219: func Log(c context.Context, err error,
excludePkgs ...string) {
is 'excludePkgs' feature used outside of unit tests for this module? I propose
to axe it. Or make excluded packages global variable populated during init time.

Mostly because errors.Log(c, err, "luci-go/stuff1", "luci-go/stuff2") looks
weird.

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:251: func (r *renderedFrame) DumpWrappersTo(w
io.Writer, from, to int) (n int, err error) {
is DumpWrappersTo part of some interface?

If not, better call it dumpWrappersTo

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:304: goID   goroutine.ID
:)

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:362: // renderedError is a series of RenderedStacks,
one for each goroutine that the
tbh, I don't understand why we want to keep rendered errors in such richly
structured form instead of rendering them directly to string buffers.

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:501: // see the text in the 'reason' field here. To
only attach an internal reason,
how do you choose when to use ' and when ` ? :) This comment uses both 'reason'
and `reason` without any apparent logic.

[iannucci, 2017-06-29 23:21:48]

Description was changed from

==========
[errors] Greatly simplify common/errors package.

Now that we have tags with values, the original concept behind the 'data'
stuff in errors no longer applies. This removes the Reason().D() system and puts
the format directly into errors.Annotate. So:
  errors.Annotate(err).Reason("foo: %(datum)s").D("datum", "value").Err()

Becomes:
  errors.Annotate(err, "foo %s", "value").Err()

This also removes a bunch of rarely-or-never-used symbols from common/errors by 
making them internal (particularly w.r.t. rendering an error). If we ever
develop
a need for them, it would be easy to bring them back.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=
==========

to

==========
[errors] Greatly simplify common/errors package.

Now that we have tags with values, the original concept behind the 'data'
stuff in errors no longer applies. This removes the Reason().D() system and puts
the format directly into errors.Annotate. So:
  errors.Annotate(err).Reason("foo: %(datum)s").D("datum", "value").Err()

Becomes:
  errors.Annotate(err, "foo %s", "value").Err()

This also removes a bunch of rarely-or-never-used symbols from common/errors by
making them internal (particularly w.r.t. rendering an error). If we ever
develop
a need for them, it would be easy to bring them back.

R=dnj@chromium.org, vadimsh@chromium.org
BUG=

Committed:
https://github.com/luci/luci-go/commit/2224066a0cc0b0fa150c733a614116c87807637e
==========

[iannucci, 2017-06-29 23:21:50]

Committed patchset #5 (id:80001) manually as
2224066a0cc0b0fa150c733a614116c87807637e (presubmit successful).

[iannucci, 2017-06-29 23:22:51]

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

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:96: //  I am an internal reson formatted with key1:
value
On 2017/06/29 02:08:31, Vadim Sh. wrote:
> typo: 'reason'
> 
> also, is this comment still up-to-date?

not really, going to make a followup CL which further simplifies this
implementation.

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:119: for key, val := range s.tags {
On 2017/06/29 02:08:31, Vadim Sh. wrote:
> we probably want to sort tags for consistent output. Getting visually
different
> error messages for same error is frustrating.

Done.

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:219: func Log(c context.Context, err error,
excludePkgs ...string) {
On 2017/06/29 02:08:31, Vadim Sh. wrote:
> is 'excludePkgs' feature used outside of unit tests for this module? I propose
> to axe it. Or make excluded packages global variable populated during init
time.
> 
> Mostly because errors.Log(c, err, "luci-go/stuff1", "luci-go/stuff2") looks
> weird.

followup, but yes, it can still be used.

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:251: func (r *renderedFrame) DumpWrappersTo(w
io.Writer, from, to int) (n int, err error) {
On 2017/06/29 02:08:31, Vadim Sh. wrote:
> is DumpWrappersTo part of some interface?
> 
> If not, better call it dumpWrappersTo

done

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:362: // renderedError is a series of RenderedStacks,
one for each goroutine that the
On 2017/06/29 02:08:31, Vadim Sh. wrote:
> tbh, I don't understand why we want to keep rendered errors in such richly
> structured form instead of rendering them directly to string buffers.

yeah we don't I'll do this in a followup

https://codereview.chromium.org/2963503003/diff/60001/common/errors/annotate....
common/errors/annotate.go:501: // see the text in the 'reason' field here. To
only attach an internal reason,
On 2017/06/29 02:08:31, Vadim Sh. wrote:
> how do you choose when to use ' and when ` ? :) This comment uses both
'reason'
> and `reason` without any apparent logic.

Done.