handler sql error #15

xuzhenglun · 2017-12-07T08:59:06Z

Now if any SQL error happens, After hook will not be called.
It's cause OpenTracing span can't be finished normally. And the span will disappear on OpenTracing dashboard(I'm using Jaeger).
It makes lot's of inconvenient because the only thing I really care about is the error message.

coveralls · 2017-12-07T09:04:13Z

Coverage increased (+2.5%) to 72.143% when pulling d7e9749 on xuzhenglun:master into b4a12ba on gchaincl:master.

keegancsmith · 2017-12-07T09:19:05Z

Related to #10

You are breaking the public API here. If we merged this PR everyones build would break. I think some point we need a nice new API to cover this use case + better performance.

but in the meantime, can I suggest changing the PR to adding a

type ErrorHook interface {
  AfterError(ctx context.Context, query string, args ...interface{}, err error) (context.Context, error)
}

then if hooks implements ErrorHook, you can call AfterError. What do you think @gchaincl?

qustavo · 2017-12-08T17:46:41Z

@keegancsmith love the ErrorHook interface it's more specific and doesn't breaks the current API.
Do we need the Before,After controls?, wouldn't just Error be enough?

keegancsmith · 2017-12-09T07:56:31Z

Don't need Before. Just calling it Error rather than AfterError is likely clear enough since I can't think of a way to report an error before anything has happened.

qustavo · 2017-12-09T21:00:04Z

@keegancsmith now I miss understood your point, I assumed that having an AfterError implied an BeforeError too. What about OnError?

xuzhenglun · 2017-12-11T06:34:10Z

Ok, I will do some changes. 😄

coveralls · 2017-12-11T09:16:50Z

Coverage decreased (-1.4%) to 68.182% when pulling 3accd84 on xuzhenglun:master into b4a12ba on gchaincl:master.

coveralls · 2017-12-11T09:16:50Z

Coverage decreased (-1.4%) to 68.182% when pulling 3accd84 on xuzhenglun:master into b4a12ba on gchaincl:master.

xuzhenglun · 2017-12-11T09:17:27Z

Should I hook the error happens in PrepareContext?
If I do so, OnError can be called before After. Is that weird?

keegancsmith · 2017-12-11T09:46:06Z

I think OnError should only ever be called on errors returned while executing the statement/query on the wrapped driver. So this is some ugly code and does a type check while inside the ExecContext impl (which isn't performant), but something like this

func (conn *ExecerContext) ExecContext(ctx context.Context, query string, args []driver.NamedValue) (driver.Result, error) {
    var err error

    list := namedToInterface(args)

    // Exec `Before` Hooks
    if ctx, err = conn.hooks.Before(ctx, query, list...); err != nil {
        return nil, err
    }

    results, err := conn.execContext(ctx, query, args)
    if err != nil {
        if errorHook, ok := conn.hooks.(ErrorHook); ok {
            if _, err2 := errorHook.OnError(ctx, err, query, list...); err2 != nil {
                return results, err2
            }
        }
        return results, err
    }

    if ctx, err = conn.hooks.After(ctx, query, list...); err != nil {
        return nil, err
    }

    return results, err
}

keegancsmith · 2017-12-11T09:47:10Z

Oh I see that is what you did, didn't notice you pushed a commit. Let me review what you have.

keegancsmith · 2017-12-11T09:48:51Z

sqlhooks.go

@@ -110,6 +119,9 @@ func (conn *ExecerContext) ExecContext(ctx context.Context, query string, args [

 	results, err := conn.execContext(ctx, query, args)
 	if err != nil {
+		if h, ok := conn.hooks.(OnError); ok {
+			ctx, err = h.OnError(ctx, err, query, list...)
+		}


I don't think you should override err. Otherwise OnError could supress errors that happen by returning nil for an error, which is likely won't most people will do.

I'd expect that if OnError did return a non-nil err, you return that error, otherwise you would return the original err.

This is a good point, while error overriding/suppressing can generate confusions it might also be helpful if you want to bypass driver specific errors. I'm not sure here.

If err is suppressed, hook must generate another results.
If both of them are nil, it is easy to panic. maybe I really need to change it to just like @keegancsmith said.

I think of Hooks as something which doesn't influence behaviour, rather observes it. We currently can mutate what ctx is, but that is more to do with it being useful to store some "request" data in it that we can then access in After. I think being able to cancel out err will lead to API which is easy to make a mistake in. Especially since we could then return nil, nil.

@keegancsmith I agree, but do you think it might be useful to suppress or modify errors?
I'm not talking about implementing it in this Hook or not, but if it's useful we might need to come up with some way to do it.

qustavo · 2017-12-11T10:10:47Z

.gitignore

@@ -22,3 +22,6 @@ _testmain.go
 *.exe
 *.test
 *.prof
+
+.idea


Do you mind to not add these?

ok, sorry for that. I will remove it.

qustavo · 2017-12-11T10:15:13Z

sqlhooks.go

@@ -110,6 +119,9 @@ func (conn *ExecerContext) ExecContext(ctx context.Context, query string, args [

 	results, err := conn.execContext(ctx, query, args)
 	if err != nil {
+		if h, ok := conn.hooks.(OnError); ok {
+			ctx, err = h.OnError(ctx, err, query, list...)
+		}


This is a good point, while error overriding/suppressing can generate confusions it might also be helpful if you want to bypass driver specific errors. I'm not sure here.

coveralls · 2017-12-11T10:37:49Z

Coverage decreased (-4.0%) to 65.625% when pulling cbccdd4 on xuzhenglun:master into b4a12ba on gchaincl:master.

qustavo · 2017-12-14T15:45:29Z

I think the question here is if Error hooks should just observe the errors or actually be able to modify it.
I like the approach @xuzhenglun has taken here, WDYT @keegancsmith ?

keegancsmith · 2017-12-14T20:47:31Z

Yeah this is the approach I think is the correct one. It technically allows you to modify the error, but if you return nil as error it returns the original error.

So all thats missing is some docstrings, but LGTM

keegancsmith · 2017-12-14T20:48:27Z

Should we bother with returning a context from OnError? It in no way affects anything

qustavo · 2017-12-15T12:39:31Z

@keegancsmith right, it's useless

coveralls · 2017-12-15T12:53:28Z

Coverage decreased (-2.7%) to 66.879% when pulling aaebe3c on xuzhenglun:master into b4a12ba on gchaincl:master.

keegancsmith

LGTM I finally gave this a proper review. Sorry for the delayed review asking for changes, but we want to merge this in and tag a new release of sqlhooks :)

If you don't have time to make these changes, I'm happy to do them.

keegancsmith · 2018-01-27T19:13:47Z

hooks/othooks/othooks.go

+		defer span.Finish()
+		span.SetTag("error", true)
+		span.LogFields(
+			log.String("error_msg", err.Error()),


There is log.Error(err) for logging an error field.

keegancsmith · 2018-01-27T19:14:36Z

hooks/loghooks/loghooks.go

@@ -28,3 +28,8 @@ func (h *Hook) After(ctx context.Context, query string, args ...interface{}) (co
 	h.log.Printf("Query: `%s`, Args: `%q`. took: %s", query, args, time.Since(ctx.Value("started").(time.Time)))
 	return ctx, nil
 }
+
+func (h *Hook) OnError(ctx context.Context, err error, query string, args ...interface{}) error {
+	h.log.Printf("Error: %v, Took: %s", err, time.Since(ctx.Value("started").(time.Time)))


Including the query and args may be useful, like we do above for After

keegancsmith · 2018-01-27T19:16:35Z

sqlhooks.go

@@ -9,12 +9,27 @@ import (
 // Hook is the hook callback signature
 type Hook func(ctx context.Context, query string, args ...interface{}) (context.Context, error)

+// ErrorHandler is the error handling callback signature
+type ErrorHandler func(ctx context.Context, err error, query string, args ...interface{}) error


Can I suggest calling this ErrorHook to be more consistent with Hook above?

keegancsmith · 2018-01-27T19:17:31Z

sqlhooks.go

 // Hooks instances may be passed to Wrap() to define an instrumented driver
 type Hooks interface {
 	Before(ctx context.Context, query string, args ...interface{}) (context.Context, error)
 	After(ctx context.Context, query string, args ...interface{}) (context.Context, error)
 }

+// ErrHook instances will be called if any error happens
+type ErrHook interface {


The idiomatic name for an interface with a single function is the function name with er suffix. EG fmt.Stringer has a String function. So in this case the interface name would be OnErrorer.

keegancsmith · 2018-01-27T19:19:27Z

sqlhooks.go

+		return e
+	}
+	return err
+}


you could simplify the below code if you took a Hook instead of an ErrHook. Then if the Hook implements the error hook interface you use it, otherwise you just return the err.

xuzhenglun · 2018-01-27T19:43:15Z

OK, I will makes those changes tomorrow. My time zone is UTC+8, I really have to sleep now.

xuzhenglun · 2018-01-30T18:35:37Z

Sorry for my delayed change because of work busy recently. Please let me know if anything Improperly.

keegancsmith

LGTM. I'll let @gchaincl take a look and merge.

xuzhenglun force-pushed the master branch from d7e9749 to 3accd84 Compare December 11, 2017 09:11

keegancsmith reviewed Dec 11, 2017

View reviewed changes

qustavo reviewed Dec 11, 2017

View reviewed changes

xuzhenglun force-pushed the master branch from 3accd84 to cbccdd4 Compare December 11, 2017 10:33

xuzhenglun force-pushed the master branch from cbccdd4 to b820a5e Compare December 15, 2017 12:49

qustavo mentioned this pull request Jan 26, 2018

Tag latest commit #16

Closed

keegancsmith reviewed Jan 27, 2018

View reviewed changes

add error handler [ErrHook]

aaebe3c

xuzhenglun force-pushed the master branch from b820a5e to aaebe3c Compare January 30, 2018 18:24

keegancsmith approved these changes Jan 30, 2018

View reviewed changes

qustavo merged commit 928fba3 into qustavo:master Jan 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

handler sql error #15

handler sql error #15

xuzhenglun commented Dec 7, 2017

coveralls commented Dec 7, 2017 •

edited

Loading

keegancsmith commented Dec 7, 2017

qustavo commented Dec 8, 2017 •

edited

Loading

keegancsmith commented Dec 9, 2017

qustavo commented Dec 9, 2017

xuzhenglun commented Dec 11, 2017

coveralls commented Dec 11, 2017

coveralls commented Dec 11, 2017

xuzhenglun commented Dec 11, 2017

keegancsmith commented Dec 11, 2017

keegancsmith commented Dec 11, 2017

keegancsmith Dec 11, 2017

qustavo Dec 11, 2017

xuzhenglun Dec 11, 2017

keegancsmith Dec 11, 2017

qustavo Dec 12, 2017 •

edited

Loading

qustavo Dec 11, 2017

xuzhenglun Dec 11, 2017

qustavo Dec 11, 2017

coveralls commented Dec 11, 2017 •

edited

Loading

qustavo commented Dec 14, 2017

keegancsmith commented Dec 14, 2017

keegancsmith commented Dec 14, 2017 •

edited

Loading

qustavo commented Dec 15, 2017

coveralls commented Dec 15, 2017 •

edited

Loading

keegancsmith left a comment

keegancsmith Jan 27, 2018

keegancsmith Jan 27, 2018

keegancsmith Jan 27, 2018

keegancsmith Jan 27, 2018

keegancsmith Jan 27, 2018

xuzhenglun commented Jan 27, 2018

xuzhenglun commented Jan 30, 2018

keegancsmith left a comment

@@ @@ -22,3 +22,6 @@ _testmain.go @@
               *.exe
               *.test
               *.prof
+              .idea

handler sql error #15

handler sql error #15

Conversation

xuzhenglun commented Dec 7, 2017

coveralls commented Dec 7, 2017 • edited Loading

keegancsmith commented Dec 7, 2017

qustavo commented Dec 8, 2017 • edited Loading

keegancsmith commented Dec 9, 2017

qustavo commented Dec 9, 2017

xuzhenglun commented Dec 11, 2017

coveralls commented Dec 11, 2017

coveralls commented Dec 11, 2017

xuzhenglun commented Dec 11, 2017

keegancsmith commented Dec 11, 2017

keegancsmith commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qustavo Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 11, 2017 • edited Loading

qustavo commented Dec 14, 2017

keegancsmith commented Dec 14, 2017

keegancsmith commented Dec 14, 2017 • edited Loading

qustavo commented Dec 15, 2017

coveralls commented Dec 15, 2017 • edited Loading

keegancsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuzhenglun commented Jan 27, 2018

xuzhenglun commented Jan 30, 2018

keegancsmith left a comment

Choose a reason for hiding this comment

coveralls commented Dec 7, 2017 •

edited

Loading

qustavo commented Dec 8, 2017 •

edited

Loading

qustavo Dec 12, 2017 •

edited

Loading

coveralls commented Dec 11, 2017 •

edited

Loading

keegancsmith commented Dec 14, 2017 •

edited

Loading

coveralls commented Dec 15, 2017 •

edited

Loading