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

Store user-defined data in Rows, Stmt and Tx objects #10

Open
fho opened this issue Aug 17, 2021 · 2 comments
Open

Store user-defined data in Rows, Stmt and Tx objects #10

fho opened this issue Aug 17, 2021 · 2 comments

Comments

@fho
Copy link
Contributor

fho commented Aug 17, 2021

Hello,

I'm implementing an interceptor with sqlmw that records traces for database
operations (instrumentedsql->sqlmw->tracing with sqlmw :-)).

When a Rows, Stmt or Tx object is created I create a parent tracing span.
Operations on the Rows, Stmt and Tx structs should be created as child spans. When
the Rows, Stmt, Tx operations is finished (Close(), Commit(), Rollback(),
etc), the parent span is also finished.

To be able to create a child span, the parent span must be available in the
methods of the Rows, Stmt and Tx objects.
One way to to achieve this would be to wrap the Rows, Stmt and Tx objects
again in my interceptor implementation and store the additional information in
the struct.

I would like to avoid having to wrap those structs again in my interceptor.
Wrapping it also requires to implement the fallback logic for the
Stmt.QueryContext() and Stmt.ExecContext() methods to their non context aware
variants again. This means maintaining another copy of
namedValueToValue.

It would be great if sqlmw would support the usecase to store user-defined
data when creating Rows, Stmt and Tx structs and access it in their methods instead.
sqlmw is doing something similiar already. When for example QueryContext() is
called, it stores the cx in the wrappedRows objects and passes it as
parameters to methods like RowsNext() as argument, which do not have a ctx
argument in the stdlib implementation.

What do you think about the idea?
Does somebody have an suggestion for how to implement it?
(I'll extend this issue or create a PR, when I have an idea.)

@inconshreveable
Copy link
Contributor

inconshreveable commented Aug 17, 2021

The stated goal of the proposal seems reasonable to me. I'm afraid I don't have the time or the context in my head to evaluate the proposed solution though, although it principle it sounds like a reasonable approach.

@fho
Copy link
Contributor Author

fho commented Aug 17, 2021

ok, thanks for the fast response nonetheless.
Maybe something else sees it and has an opinion or even a better idea.

I experimented a bit more in the meantime:
The Rows and Tx objects currently can be simply wrapped by the middleware and the parent Rows, Tx embedded.
In the related interceptor methods (RowsNext, RowsClose, TxCommit, etc), a type check can be done if the passed rows or Tx is the own wrapped type.
This works fine and the wrapping overhead is also negligible.

For statements the approach does not work.
sqlmw wraps the driver.Stmt that is returned by the interceptors ConnPrepareContext method again into a sqlmw.wrappedStmt. Because that type is private the user can not convert it to it's own wrapped stmt type in the middleware anymore.
Another difficulty with Stmts is that ConnPrepareContext returns a driver.Stmt interface.
The interceptor methods operating on more specific interfaces like driver.QueryerContext, driver.ExecerContext.
To be able to handle that, the user has to check in ConnPrepareContext which interfaces the passed driver.ConnPrepareContext actually implement and then wraps it in objects that have the same interfaces.
Currently there are 3 interfaces that extends the Stmt in the stdlib, all combinations of those must be handled.
Alternatively the same then sqlmw.WrappedParentStatement must be implemented in the middleware again.

Possible Solutions:

  • make sqlmw.WrappedParentStmt public and change the parameter type of stmt in StmtQueryContext and StmtExecContext to sqlmw.WrappedParentStmt. A middleware can then extract it's own wrapped statement. This would be a simple change in sqlmw, the user would still have to cope with all the different Stmt interfaces though.
    (Independent of this issue I think this change would still make sense to clarify the interface. Currently it is hidden, that the statement interceptor methods do not get passed the same stmt that the user returned in PrepareStatement.)
  • Support to store user-defined data in Stmts, Tx, Rows in the following way:
    A type UserData struct { atomic.Value } is introduced.
    Interceptor methods that return Rows, Tx and Stmts are changed to have an
    additional *UserData parameter.
    In the interceptor methods Userdata.Store() can be called to store custom
    data for Rows, Tx or Stmts.
    sqlmw stores the UserData in the wrapped Rows, Tx, Stmts.
    The data can be in RowsNext(), StmtQueryContext(), TxCommit(), etc
    retrieved via helper functions like:
    func GetUserDataRows(driver.Row) interface{} {}, func GetUserDataStmts(driver.Stmt) interface{} {}, func GetUserDataTx(driver.Tx) interface{} {}.

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

Successfully merging a pull request may close this issue.

2 participants