Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/pkgsite: improve error reporting #44231

Closed
jba opened this issue Feb 12, 2021 · 7 comments
Closed

x/pkgsite: improve error reporting #44231

jba opened this issue Feb 12, 2021 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. pkgsite
Milestone
pkgsite/unplanned

Comments

@jba
Copy link
Contributor

jba commented Feb 12, 2021

Currently, we call the GCP ErrorReporting API as middleware whenever we see a 5xx response. This has a couple of problems:

  1. Since it's middleware, we don't have the actual error, just the nested handler's HTTP response.
  2. The middleware runs near the top of the stack, so the stack traces for all errors are the same.
  3. To mitigate (1) and (2) we've been reporting lower down in the stack, but that results in double-reporting, since nothing disables the error-reporting middleware.
@jba jba added NeedsFix The path to resolution is known, but the work has not been done. pkgsite labels Feb 12, 2021
@jba jba added this to the pkgsite/unplanned milestone Feb 12, 2021
@jba jba self-assigned this Feb 12, 2021
@gopherbot
Copy link

Change https://golang.org/cl/291489 mentions this issue: internal/middleware: allow bypassing error reporting

@gopherbot
Copy link

Change https://golang.org/cl/291490 mentions this issue: internal/derrors: add stack trace

@gopherbot
Copy link

Change https://golang.org/cl/291491 mentions this issue: internal/frontend: report errors

@gopherbot
Copy link

Change https://golang.org/cl/291492 mentions this issue: internal/postgres: add a stack to all wrapped errors

@gopherbot
Copy link

Change https://golang.org/cl/291549 mentions this issue: internal/worker: report fetch errors

@gopherbot
Copy link

Change https://golang.org/cl/291550 mentions this issue: internal/proxy: add stack traces to wrapped errors

gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 12, 2021
@jba
internal/middleware: allow bypassing error reporting
bbaec8b 
Use a header to communicate to the error-reporting middleware that an
error report should not be sent.

This can be used to address item (3) on the issue, duplicate error
reporting.

For golang/go#44231

Change-Id: I33e91fe6b7af39bf2a2221fe20b16100b956a9e6
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/291489
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 12, 2021
@jba
internal/derrors: add stack trace
23a5004 
Add StackError, an error with a stack trace.

Add the WrapStack function to add one while wrapping.

We can use these to capture a stack lower down for
later reporting, when we have more context.

For golang/go#44231

Change-Id: Ib00aa75daa46e60ced99bfa5f1fd65bb31828d01
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/291490
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 12, 2021
@jba
internal/frontend: report errors
d4a5434 
Call the error reporting service when the frontend
is about to serve an error. At this point we know
the request and the error.

If the error has a stack trace, add it.

Use the newly added header to bypass the error-reporting middleware,
avoiding duplicate reports.

For golang/go#44231

Change-Id: I09bec6ffeccb1624d6378e614e1209f9630ab419
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/291491
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 12, 2021
@jba
internal/postgres: add a stack to all wrapped errors
1bbbd6c 
Add a stack trace when we wrap an error from the DB.

These traces can be sent to the error reporting service.

For golang/go#44231

Change-Id: I096cdec4e97a6dcb0b7eb2ccdb4c955e1a0f4ccd
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/291492
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 12, 2021
@jba
internal/worker: report fetch errors
ab91b7d 
Send errors from fetching to the error reporting service, bypassing
the middleware.

For golang/go#44231

Change-Id: Iafdfcfa9950dacf9bbef732f1ae5644edda6b384
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/291549
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
gopherbot pushed a commit to golang/pkgsite that referenced this issue Feb 12, 2021
@jba
internal/proxy: add stack traces to wrapped errors
7015347 
For golang/go#44231

Change-Id: Ia285ee6f2493180727198cf9a1684f8d32fe2e64
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/291550
Trust: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Julie Qiu <julie@golang.org>
@jba
Copy link
Contributor Author

jba commented Feb 18, 2021

We added stacks to errors, and moved the error-reporting call to a place where both the error and the request are available.

There is potentially more incremental work to do, like adding stack traces along more paths, but this is essentially done.

@jba jba closed this as completed Feb 18, 2021
@golang golang locked and limited conversation to collaborators Feb 18, 2022
@rsc rsc unassigned jba Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. pkgsite
Projects
None yet
Milestone
pkgsite/unplanned
Development

No branches or pull requests
2 participants
@gopherbot @jba