Opentelemetry for database/sql

[wingyplus]

It's good to see support for database/sql driver. We've some discussion here open-telemetry/opentelemetry-go#490.

[andrewhsu]

FYI initial attempt that looks like a different approach would be more desirable #32

[seslattery]

I have an initial proof of concept I implemented at https://github.com/seslattery/otelsql. If this seems like a useful direction to be explored more, I'd be happy to open a PR and work on polishing it up some more.

[Aneurysm9]

@seslattery please do open a PR. I'd like to see Register() use functional options for the tracer and attributes, to be more in line with the SDK and other instrumentations. I'm also not sure about using runtime.Caller() to derive span names from callers rather than just putting static strings at the call site. Overall, though, this is a good start.

[MrAlias]

Prior work in OpenCensus might be useful to look at: https://github.com/opencensus-integrations/ocsql

I would not copy this, but might be interesting to look at.

[MrAlias]

Also: https://github.com/DataDog/dd-trace-go/tree/v1.24.1/contrib/database/sql

[MrAlias]

Prior attempt within this repo: #32

[seanschade]

@seslattery are you still planning on submitting a PR?

I have an initial proof of concept I implemented at https://github.com/seslattery/otelsql. If this seems like a useful direction to be explored more, I'd be happy to open a PR and work on polishing it up some more.

@MrAlias what are your thoughts on using something like https://github.com/ngrok/sqlmw as an interceptor to wrap drivers?

I think an approach similar to https://github.com/seslattery/otelsql is more desirable than #32.

If we would like to forego the wrapper approach then we have both OpenTracing and OpenCensus examples to draw from.

It would be nice if we could enumerate what we like about each, and what this should look like for OpenTelemetry.

[seslattery]

@seanschade Thanks for the ping! Sorry for the delay, this fell off my radar. I cleaned up the code a little bit and added an example. PR is here: #320. Notably my implementation still doesn't have any tests.

[derekperkins]

Looking at that PR, it would be nice to be able to opt into specific spans. The SQL driver can be very chatty, so allowing opt-in like the ocsql package would be nice.
https://github.com/opencensus-integrations/ocsql/blob/master/options.go#L12-L64

[MrAlias]

@MrAlias what are your thoughts on using something like https://github.com/ngrok/sqlmw as an interceptor to wrap drivers?

I agree with @derekperkins on the approach. I think an approach similar to what OpenCensus did is preferred to the approach of using the sqlmw approach.

It would be nice if we could enumerate what we like about each, and what this should look like for OpenTelemetry.

This sounds like a great idea 👍 . I think including something like this in this issue would help future developers understand the eventual choice made and what out strategy was.

[j2gg0s]

Is there any progress about this issue?
If we need an approache like opencensus, how about https://github.com/j2gg0s/otsql , used in our other project.

[kevindavee]

hi, any progress about this issue?

[MrAlias]

Is there someone is this issue that could head up this effort? This issue has not seen much focus from the maintainer/approver development team of this project as it is not a part of the things needed for our GA release (there are quite a lot of issues prioritized for that effort that need to take precedence).

That said, it seems like there is a fair amount of community interest here. It would be ideal if someone can take the initiative and start work on what @seanschade laid out as a first step to a solutions design:

I think an approach similar to https://github.com/seslattery/otelsql is more desirable than #32.

If we would like to forego the wrapper approach then we have both OpenTracing and OpenCensus examples to draw from.

It would be nice if we could enumerate what we like about each, and what this should look like for OpenTelemetry.

From the outcome of that analysis the implementation of a solution would need to be iterative. We've discussed a few times in our SIG meetings that the initial solution here does not need to solve all the combinatorial possibilities (like #32) but provided more of an MVP to build from.

[shousper]

+1 for @j2gg0s's implementation. otelsql only has a tracing implementation.

Metrics are far more important to us than tracing as they power alerting, dashboards and automation.

[seanschade]

+1 for @j2gg0s's implementation. otelsql only has a tracing implementation.

Metrics are far more important to us than tracing as they power alerting, dashboards and automation.

This solution would meet my needs. Could @j2gg0s solution be the MVP, and would it move into the contrib repo?

[dackroyd]

Are alternative designs being considered? ocsql, otelsql (which is based on that), and instrumentedsql (OpenTracing option, e.g. https://github.com/luna-duclos/instrumentedsql) are all very verbose in the number of spans that are created (as @derekperkins pointed out), most of which are of low value by themselves (e.g. 'rows next'). While it is possible to disable creation of spans for specific things, instrumenting this way may not be the most effective way to make things observable?

Using an observability tool like Honeycomb/Lightstep, you want as many details available on the one span as possible as that allows those things to be properly queried, and expose where other tags differ on those kinds of spans. The way I imagine that would be something like:

name: "SELECT"
tags:
  # Use proper query text per OTel spec
  db.statement: "SELECT ... FROM ... WHERE ..."
  # Statement duration before iterating over rows
  db.statement_duration_ms: 38.2
  # Need to be considered carefully, given args may include sensitive data like PII, or could have large values
  db.arg.foo: "bar"
  db.arg.fizz: "buzz"
  # Could be accumulated as the calling code iterates over the rows, added on `rows.Close()`
  db.rows_returned: 23
  ...
  # Spec tags that apply to all DB spans
  db.system: "postgresql"
  db.user: "username"
  db.name: "Customers"
  ...

I'd also be looking to hook into these details to aggregate things like number of statements executed, rows returned, total duration to put into the root span for the service e.g. the HTTP request that needs those queries executed.

[MrAlias]

From the SIG meeting today:

@Aneurysm9 mentioned that possibly Honeycomb might be able to contribute their DB instrumentation so it could be converted and used as OTel instrumentation. @Aneurysm9 is going to follow up with @lizthegrey.

Also,

As there are multiple approaches to provide this instrumentation we should likely add multiple instrumentation packages for users to try. Ideally we will ultimately provide a single well vetted and supported instrumentation library for the db package, but until then we should add add an experimental directory and add all 3 (possibly 2) solutions listed here to that directory so they can be iterated on and users can evaluate them.

[chainlink]

Any update on the SIG discussion?

[XSAM]

I'd like to work on this by implementing an MVP (probably not include metrics first)

[derekperkins]

I've been watching this issue, but I wasn't aware that @XSAM had indeed built a version https://github.com/XSAM/otelsql. In his PR, @Aneurysm9 said this about landing official support #505 (comment)

We've discussed this a couple times at the SIG meeting and the consensus seems to be that we are not ready to land a db/sql instrumentation in the contrib repo at this time. We would like to see broader usage and community consensus on an approach before we commit to the level of support that would be required of a package in contrib.

[lizthegrey]

cc @bdarfler @adamopenweb

[MrAlias]

In accordance with the new instrumentation policy hosting guidelines I'm closing this.

[derekperkins]

I'm not 100% sure I understand why this wouldn't be hosted here. I get offloading instrumentation for public packages to their repos, but I don't see how that applies to stdlib packages. This database/sql package is specifically called out there, with this as the required import path. go.opentelemetry.io/contrib/instrumentation/database/sql/otelsql Can anyone create a standalone repo to claim that import path? It seems to me that any stdlib instrumentation should live here in the contrib repo.

[lizthegrey]

+1 to the idea that if net/http lives here, then so should database/sql

[derekperkins]

@MrAlias any feedback on this?

[derekperkins]

For anyone subscribed to or who finds this issue, this fork https://github.com/XSAM/otelsql is active and probably your best bet for the time being.

[tonglil]

Has anyone done a comparison between https://github.com/XSAM/otelsql and https://github.com/uptrace/opentelemetry-go-extra/ ?