gateway: degrade error in gateway to log to reduce noise by Kubuxu · Pull Request #2971 · ipfs/kubo

Kubuxu · 2016-07-15T11:34:28Z

It logs all errors including expired IPNS keys and other non important
errors.

License: MIT
Signed-off-by: Jakub Sztandera kubuxu@protonmail.ch

RichardLitt · 2016-07-15T12:09:11Z

core/corehttp/gateway_handler.go

 func webErrorWithCode(w http.ResponseWriter, message string, err error, code int) {
 	w.WriteHeader(code)
-	log.Errorf("%s: %s", message, err) // TODO(cryptix): log errors until we have a better way to expose these (counter metrics maybe)
+	log.Infof("%s: %s", message, err) // TODO(cryptix): log until we have a better way to expose these (counter metrics maybe)


Can we split out this TODO into an issue?

ghost · 2016-07-15T13:40:06Z

I think this is probably insufficient. Some of these are legitimate errors, some are just totally normal from the gateway's perspective. Error handling in the gateway generally needs to be reworked.

ghost · 2016-07-15T13:40:22Z

But 👍 LGTM, since it reduces log noise.

Kubuxu · 2016-07-15T13:40:38Z

It does and that is what #2972 is for.

whyrusleeping · 2016-07-15T14:00:46Z

I agree with @lgierth here, i dont think this is what we want to do. We should filter out explictly which errors we want to demote, this was here for a reason, we want to know when something has gone wrong and handle it accordingly.

Kubuxu · 2016-07-16T11:47:05Z

Reworked it.

whyrusleeping · 2016-08-04T14:10:42Z

core/corehttp/gateway_handler.go

 		w.WriteHeader(http.StatusServiceUnavailable)
 		fmt.Fprint(w, "Could not resolve path. Node is in offline mode.")
 		return
+	} else if err == namesys.ErrResolveFailed {


lets change this over to an error switch

whyrusleeping · 2016-08-04T14:11:52Z

We might also want to handle context deadline exceeded errors (i see these somewhat often too) and add some tests.

jbenet · 2016-08-04T21:28:42Z

+1 to being very explicit about what's getting demoted and making
absolutely sure it's not a problematic condition users should be made aware
of. (What @lgierth said)
On Thu, Aug 4, 2016 at 10:11 Jeromy Johnson notifications@github.com
wrote:

We might also want to handle context deadline exceeded errors (i see these
somewhat often too) and add some tests.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#2971 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAIcoT8XDsvUA9qMT0kegwGzEp0qUHpAks5qcfMpgaJpZM4JNUN_
.

Kubuxu · 2016-09-08T09:28:55Z

Updated.

whyrusleeping · 2016-09-08T20:48:48Z

core/corehttp/gateway_handler.go

+	case nil:
+		// core.Resolve worked
+	case core.ErrNoNamesys:
+		if !i.node.OnlineMode() {


What happens if we get ErrNoNamesys and we're 'online' ? Even if this can never happen we should still check that case here and log an error

Kubuxu · 2016-09-12T11:24:50Z

Updated

Kubuxu · 2016-10-03T15:56:32Z

@lgierth can you CR on this?

whyrusleeping · 2016-10-04T01:32:13Z

LGTM, just waiting on a 👍 from @lgierth

ghost · 2016-10-04T09:22:30Z

core/corehttp/gateway_handler.go

+		// core.Resolve worked
+	case namesys.ErrResolveFailed:
+		// Don't log that error as it is just noise
+		w.WriteHeader(http.StatusBadRequest)


400 doesn't fit here since the 4xx range is for client errors, where the client can amend the request to possibly get a successful response. 502 Bad Gateway or, (less specific) 500 Internal Server Error would be sufficient.

ErrResolveFailed means that the link passed by client can't be resolved, this doesn't mean it is server fault that it can't be resolved.

Poor server, we're blaming everything on it :P The meaning of a 4xx for the client is "don't retry unless you make changes to the request" though, and the reason for ErrResolveFailed can be an invalid /ipns string, or a failure in DNS resolution.

Anyhow, I don't have strong feelings about it right now, because the 400 status here doesn't regress from what we already have. We obviously need more granular errors through the stack though.

ghost · 2016-10-04T09:31:10Z

core/corehttp/gateway_handler.go

+		fallthrough
+	default:
+		// all other erros
 		webError(w, "Path Resolve error", err, http.StatusBadRequest)


I'm okay to leave this existing line with 400 for now, but the same applies here.

Kubuxu · 2016-10-04T16:26:27Z

@lgierth updated

ghost

Thanks LGTM 👍

Kubuxu · 2016-10-05T05:18:49Z

Wait, it broke the tests...

Kubuxu · 2016-10-24T16:07:27Z

@lgierth can you confirm change in behaviour here:

--- FAIL: TestGatewayGet (0.01s)
    gateway_test.go:127: http://127.0.0.1:52637
    gateway_test.go:156: got 500, expected 400 from http://localhost:5001/ipns/nxdomain.example.com

It logs all errors including expired IPNS keys and other non important errors. License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

ghost · 2016-11-16T04:38:31Z

This LGTM 👍, I went and resolved the conflicts with the recent coreapi changes, @Kubuxu I thought that's likely okay :)

Sorry for sitting on it so long!

Kubuxu force-pushed the feature/gateway-resolve-error branch from 1391440 to c1f7428 Compare July 15, 2016 11:37

RichardLitt reviewed Jul 15, 2016
View reviewed changes

Kubuxu mentioned this pull request Jul 15, 2016

gateway: better error exposing in logs from gateway code #2972

Open

Kubuxu added the RFM label Jul 15, 2016

whyrusleeping removed the RFM label Jul 15, 2016

Kubuxu force-pushed the feature/gateway-resolve-error branch from c1f7428 to 919d835 Compare July 16, 2016 11:41

Kubuxu added the need/review Needs a review label Jul 16, 2016

whyrusleeping reviewed Aug 4, 2016
View reviewed changes

whyrusleeping added need/author-input Needs input from the original author and removed need/review Needs a review labels Aug 4, 2016

whyrusleeping assigned Kubuxu Aug 26, 2016

Kubuxu force-pushed the feature/gateway-resolve-error branch from 919d835 to 51e55d7 Compare September 8, 2016 09:14

Kubuxu added need/review Needs a review and removed need/author-input Needs input from the original author labels Sep 8, 2016

Kubuxu removed their assignment Sep 8, 2016

Kubuxu mentioned this pull request Sep 8, 2016

Enabling bloom filter ipfs/infra#174

Closed

whyrusleeping reviewed Sep 8, 2016
View reviewed changes

Kubuxu self-assigned this Sep 9, 2016

Kubuxu force-pushed the feature/gateway-resolve-error branch from 20d4bf5 to bba2a85 Compare September 12, 2016 11:24

Kubuxu removed their assignment Sep 12, 2016

Kubuxu force-pushed the feature/gateway-resolve-error branch from bba2a85 to 9124d2e Compare September 12, 2016 11:25

whyrusleeping added the status/in-progress In progress label Sep 14, 2016

ghost suggested changes Oct 4, 2016

View reviewed changes

ghost approved these changes Oct 4, 2016

View reviewed changes

ghost mentioned this pull request Oct 5, 2016

gateway: use core api for serving GET/HEAD/POST #3244

Merged

whyrusleeping assigned Kubuxu Oct 6, 2016

Kubuxu assigned ghost and unassigned Kubuxu Oct 24, 2016

ghost added the topic/gateway Topic gateway label Oct 28, 2016

whyrusleeping added status/ready Ready to be worked and removed status/in-progress In progress labels Nov 2, 2016

Kubuxu added 2 commits November 16, 2016 05:25

gateway: degrade error in gateway to log to reduce noise

82d46a5

It logs all errors including expired IPNS keys and other non important errors. License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

gateway: use switch for error handling

43320e1

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

ghost force-pushed the feature/gateway-resolve-error branch from 1ad08da to 63ea995 Compare November 16, 2016 04:29

gateway: change status code of failed namesys resution to 500

ce96b91

License: MIT Signed-off-by: Jakub Sztandera <kubuxu@protonmail.ch>

ghost force-pushed the feature/gateway-resolve-error branch from 63ea995 to ce96b91 Compare November 16, 2016 04:37

whyrusleeping merged commit 4cfd841 into master Nov 16, 2016

whyrusleeping deleted the feature/gateway-resolve-error branch November 16, 2016 09:00

whyrusleeping removed the status/ready Ready to be worked label Nov 16, 2016

ghost mentioned this pull request Dec 23, 2016

Code Review v0.4.4...v0.4.5 #3534

Closed

Uh oh!

Conversation

Kubuxu commented Jul 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RichardLitt Jul 15, 2016

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 15, 2016

Uh oh!

ghost commented Jul 15, 2016

Uh oh!

Kubuxu commented Jul 15, 2016

Uh oh!

whyrusleeping commented Jul 15, 2016

Uh oh!

Kubuxu commented Jul 16, 2016

Uh oh!

whyrusleeping Aug 4, 2016

Choose a reason for hiding this comment

Uh oh!

whyrusleeping commented Aug 4, 2016

Uh oh!

jbenet commented Aug 4, 2016

Uh oh!

Kubuxu commented Sep 8, 2016

Uh oh!

whyrusleeping Sep 8, 2016

Choose a reason for hiding this comment

Uh oh!

Kubuxu commented Sep 12, 2016

Uh oh!

Kubuxu commented Oct 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whyrusleeping commented Oct 4, 2016

Uh oh!

ghost Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

Kubuxu Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

Kubuxu commented Oct 4, 2016

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Kubuxu commented Oct 5, 2016

Uh oh!

Kubuxu commented Oct 24, 2016

Uh oh!

ghost commented Nov 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Kubuxu commented Jul 15, 2016 •

edited

Loading

Kubuxu commented Oct 3, 2016 •

edited

Loading

Kubuxu Oct 4, 2016 •

edited

Loading