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

Implement Go version of SQL Commenter #101

Closed
wants to merge 1 commit into from

Conversation

jschaf
Copy link

@jschaf jschaf commented Mar 21, 2022

This implements the SQL Commenter specification for affixing comments to a
SQL query. Loosely inspired by url.Values 1 in the Go standard library.

The specification doesn't fully describe the following situations, so I'll
list the intended behavior of the Go implementation here:

  • Empty keys: omitted and don't appear in the comment string.

  • Keys with single quotes: this Go library uses the valid URL encoding
    of a single quote %27 instead of escaping with a backslash. The
    spec has the confusing example: name'' serialized to name=\'\'.
    Where did the equal sign come from? Also, the website shows fancy
    quotes, not plain ascii quotes.

  • Already URL encoded values: this Go library double-encodes the value,
    meaning the key-pair a=%3D is encoded as a='%253D'. The spec exhibit
    doesn't double-encode, instead preserving the original percent encoding.
    That seems inadvisable since the parsing a serialized sql comment won't
    result in the same original values.

Fixes #95.

This implements the SQL Commenter specification for affixing comments to a
SQL query. Loosely inspired by url.Values [1] in the Go standard library.

The specification doesn't fully describe the following situation, so I'll
list the intended behavior of the Go implementation here:

- Empty keys: omitted and don't appear in the comment string.

- Keys with single quotes: this Go library uses the valid URL encoding
  of a single quote `%27` instead of escaping with a backslash. The
  spec has the confusing example: `name''` serialized to `name=\'\'`.
  Where did the equal sign come from? Also, the website shows fancy
  quotes, not plain ascii quotes.

- Already URL encoded values: this Go library double-encodes the value,
  meaning the key-pair `a=%3D` is encoded as `a='%253D'`. The spec exhibit
  doesn't double-encode, instead preserving the original percent encoding.
  That seems inadvisable since the parsing a serialized sql comment won't
  result in the same original values.

Fixes google#95.

[1]: https://pkg.go.dev/net/url#Values
@sjs994
Copy link
Collaborator

sjs994 commented Mar 29, 2022

Thanks @jschaf for the PR. Will take a look today.

@sjs994
Copy link
Collaborator

sjs994 commented Mar 29, 2022

So, we are mapping a map of values to the SQLCommenter format.
IMU It's more like a manual instrumentation setup as of now.

Are we planning to add auto-instrumentation after this ?

If we look at other sqlcommenter libraries (js-knex example), we are trying to achieve auto instrumentation i.e user need to just setup the library. Then the library (a set of middleware) will fetch the information like route and pass that to the SQLComment.

@jschaf
Copy link
Author

jschaf commented Mar 31, 2022

Are we planning to add auto-instrumentation after this ?

There's at least two things to make auto-instrumentation work for HTTP requests:

  1. Create an http.Handler that injects the sqlcommenter.Values into the context. This is pretty straight-forward. If you want otel integration, it would require pulling in https://pkg.go.dev/go.opentelemetry.io/otel/api/trace#SpanFromContext to get the trace ID and span ID which includes 5 dependencies.

  2. A custom db/sql driver that can modify the SQL query to include the sqlcommenter integration. I'd probably pull in https://github.com/qustavo/sqlhooks to do it since that code looks like a pain to implement. One problem is that library depends on lib/pq, go-sqlite3, and go-sql-driver/mysql but only for tests which means it's a rather heavyweight dependency. https://deps.dev/go/github.com%2Fqustavo%2Fsqlhooks%2Fv2/v2.1.0/dependencies

Another issue is that lib/pq is deprecated for Postgres in favor of pgx. I think the only way to hook into a pgx.Conn is to wrap it, like option 1 in https://anymindgroup.com/news/tech-blog/15724/

To put my cards on the table, I only need the PR I've implemented since we have our own wrapper around pgx so I'm not likely to pick up any additional work. If that's a deal-breaker let me know and I can close this and someone can use it in the future for an auto-instrumented implementation.

@artefactop
Copy link
Contributor

Hi, I also interested in the Go version for sqlcommenter, I discover that ent already has support for it, maybe the integration should be similar:

  • Create db driver
  • Adding context in the middleware

https://github.com/ariga/sqlcomment

@sjs994
Copy link
Collaborator

sjs994 commented Apr 7, 2022

@jschaf I am thinking there is a value add to add this code as a library (go/sqlcommenter/lib/**) instead. That way , we can use this on implementing the go auto-instrumentation sqlcommenter library.

What do you think ?

Thanks @artefactop, thats a good starting point to look. Will explore this.

@hslatman
Copy link

hslatman commented Apr 7, 2022

An alternative to the sqlhooks library is to use https://github.com/ngrok/sqlmw. It's more lightweight in terms of dependencies it pulls in.

@hedwigz
Copy link

hedwigz commented May 24, 2022

Hey guys, I am the author of Ent's sqlcomment.
sqlcomment works by decorating the Ent's SQL driver and already supports OpenCensus and OpenTelemetry so I think we can push it one level lower to the SQL driver level as @hslatman said there's sqlmw which can fit.
WDYT?

@sjs994
Copy link
Collaborator

sjs994 commented May 28, 2022

Thanks @hedwigz! Sorry to miss this message.
We did not get much time to try out a POC.

We are planning to start brainstorming about go-sqlcommenter after 2nd week of June. We will look at sqlmw then.

Maybe we will start a discussion on an issue in this repository. Will pull you guys there. Would that be fine ?

@jschaf
Copy link
Author

jschaf commented May 29, 2022

@jschaf I am thinking there is a value add to add this code as a library (go/sqlcommenter/lib/**) instead. That way , we can use this on implementing the go auto-instrumentation sqlcommenter library.

I'm happy to move this wherever, but I think something like the following would be more idiomatic:

  • sqlcommenter/
    • values.go (with Values struct)
    • sqlcommenter_ent/
    • sqlcommenter_pgx/

@jschaf jschaf closed this Jul 15, 2022
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 this pull request may close these issues.

Support Go language
5 participants