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

sql.DB Context support #258

Open
arbarlow opened this issue Feb 6, 2018 · 10 comments
Open

sql.DB Context support #258

arbarlow opened this issue Feb 6, 2018 · 10 comments

Comments

@arbarlow
Copy link

arbarlow commented Feb 6, 2018

Hi there,

Love the idea behind Kallax and I'm happy to start contributing, I'm opening an issue first to discuss and can then PR if need be!

I'm wondering if there is/have been plans for Go's new sql.DB Context support?
golang/go#15123

I rely on this heavily for distributed tracing

@roobre
Copy link
Contributor

roobre commented Feb 6, 2018

Hello!

Thanks for your interest in kallax :D

Regarding the conext support, I'm not sure how is this related to kallax. Could you explain a little more about where this could be added and what usages it would have?

@arbarlow
Copy link
Author

arbarlow commented Feb 10, 2018

Sure,

It allows you to pass a context to sql.DB methods, so that you can hook into that for passing along whatever you need, be it distributed tracing information or a user token etc.

For example, this library allows for sending traces to Google or for logging. https://github.com/ExpansiveWorlds/instrumentedsql/blob/master/sql_example_test.go

And here is how we send traces to Zipkin, https://github.com/echo-health/opentracing-gorm/blob/master/otgorm.go

So, whilst you might want to supply context with every method, another way is to have a method to attach a context and get back a copy of the store of similar. Or work much like the store.Transaction method works currently in that provide a function, but that wouldn't be my personal preference.

@Azuka
Copy link

Azuka commented Mar 14, 2018

Hi, just wanted to see if you were considering supporting this. Ideally the hook interface methods would take in a context, although that would be a breaking change.

Edit: That'd only be an addition. I noticed you're using squirrel underneath for query generation, which has context support. allowing context to be passed in would bring in all the advantages of contexts such as deadline support. I understand this would possibly impact both the code generation as well as reorganization of internal methods to reduce duplication. Is there any way I can help?

@clintberry
Copy link

Beyond tracing and user tokens, it primarily supports timeouts which propagate to sql.DB. This means if you set a time limit on your request and it isn't going to make it, sql.DB will honor that and cancel the query so as to not waste anymore postgres resources on a query you don't want anymore.

@giautm
Copy link

giautm commented Oct 23, 2018

Sure, I'm looking for this feature too.

@giautm
Copy link

giautm commented Nov 15, 2018

Any ETA on this??? Go's Context released for a long time (a year ago), support context.Context brings a lot of benefits, such as cancel query, logging, analyst, and multi-tenant support.

@roobre
Copy link
Contributor

roobre commented Nov 15, 2018

Hello, thanks for your interest. For now, not in the near future, as I don't currently have the time need to implement and test new features. Pull requests are welcome though.

@giautm
Copy link

giautm commented Nov 15, 2018

👍 , I need some help to know the architecture of this project. For a quick look, we will change Store implement, and generator to deliver this feature?

@roobre
Copy link
Contributor

roobre commented Nov 15, 2018

Changes would need to be non-breaking. Probably something like we have for running with proxies: A .WithContext() method on Store which would return a store (or ContextStore, if not compatible) using a context for their operations.

@giautm
Copy link

giautm commented Nov 15, 2018

Thanks for the suggestion, squirrel.DBProxyContext doesn't support context yet (receivers: ExecContext, QueryContext...), so we will switch to an internal interface. Hừm...

// Store is a structure capable of retrieving records from a concrete table in
// the database.
type Store struct {
	db        squirrel.DBProxyContext
	runner    squirrel.DBProxyContext
	useCacher bool
	logger    LoggerFunc
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants