Add tracing instrumentation for database/sql

[XSAM]

Related #5

Here is the MVP instrumentation of database/sql.

After I looked at some of the prior implementations: https://github.com/j2gg0s/otsql, https://github.com/opencensus-integrations/ocsql, and https://github.com/DataDog/dd-trace-go/tree/v1.24.1/contrib/database/sql, I find out most of the codes from these arts are similar. Also, they all use Register to create a new SQL driver.

The big difference is whether to add extra spans, for example, creating spans in the Ping function. I accept some features and drop the rest. Here is the detail:

Feature	Description	Status	Reason
Ping	If set to true, will enable the creation of spans on Ping requests.	Accepted	Ping has context argument, but it might no needs to record.
AllowRoot	If set to true, will allow ocsql to create root spans in absence of existing spans or even context.	Dropped	I don't think traces data have meaning without context.
Rows, RowsClose	If set to true, will enable the creation of spans on corresponding calls.	Accepted as a basic feature	We need to know the status of Rows
RowsNext	If set to true, will enable the creation of events on corresponding calls. This can result in many events.	Accepted	It provides more visibility.
RowsAffected, LastInsertID	If set to true, will enable the creation of spans on RowsAffected/LastInsertId calls.	Dropped	Don't know its use cases. We might add this later based on the users' feedback.
Query	If set to true, will enable recording of sql queries in spans.	Accepted as a basic feature	db.statement will use this, which is a required attribute. https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/database.md
QueryParams	If set to true, will enable recording of parameters used with parametrized queries.	Dropped	It will cause high cardinality values and security problems.
DisableErrSkip	If set to true, will suppress driver.ErrSkip errors in spans.	Accepted	ErrSkip error might annoying

Although I choose to drop these features, we might add them in the future; this is just an MVP implementation.

[XSAM]

Before adding tests, @open-telemetry/go-approvers I'm interested in your opinions on this.

[nvx]

The rationale for the Rows* spans is that if you do a query that returns a significant number of rows, the actual query will likely return quickly, but fetching the rows may take a non-insignificant amount of time. From looking at the current implementation this would be invisible to the tracing.

While creating an unbounded number of spans is definitely a bad idea, not having visibility is also less than ideal. I think a model similar to the net/http tracing where a parent span is created that persists for the entire duration with sub-spans for the query itself and RowsClose/RowsAffected calls and instead of spans for RowsNext perhaps use a log entry to indicate when it was called as this is a lot lighter weight than a span for it. The parent span would finish when RowsClose is called.

A similar pattern would likely be useful for transactions too with the parent persisting until Commit or Rollback is called (although context propagation in this instance might be a bit trickier so maybe this is less doable).

[XSAM]

@nvx Thanks for the review. I made some changes to record the status of Rows by creating a new span, PTAL.

[codecov]

Codecov Report

Merging #505 (abcc58d) into main (f82555b) will increase coverage by 0.1%.
The diff coverage is 81.2%.

@@           Coverage Diff           @@
##            main    #505     +/-   ##
=======================================
+ Coverage   77.9%   78.1%   +0.1%     
=======================================
  Files         55      65     +10     
  Lines       2602    2890    +288     
=======================================
+ Hits        2029    2259    +230     
- Misses       443     491     +48     
- Partials     130     140     +10     

Impacted Files	Coverage Δ
detectors/gcp/gce.go	7.3% <0.0%> (ø)
detectors/gcp/gke.go	0.0% <0.0%> (ø)
...ion/github.com/Shopify/sarama/otelsarama/option.go	100.0% <ø> (ø)
...ation/github.com/astaxie/beego/otelbeego/common.go	100.0% <ø> (ø)
...ation/google.golang.org/grpc/otelgrpc/grpctrace.go	58.3% <40.0%> (-8.4%)	⬇️
instrumentation/database/sql/otelsql/rows.go	51.3% <51.3%> (ø)
instrumentation/net/http/otelhttp/handler.go	72.6% <75.0%> (ø)
instrumentation/database/sql/otelsql/conn.go	76.9% <76.9%> (ø)
instrumentation/database/sql/otelsql/sql.go	80.0% <80.0%> (ø)
instrumentation/database/sql/otelsql/stmt.go	80.0% <80.0%> (ø)
... and 40 more

[XSAM]

Tests added. @open-telemetry/go-approvers, PTAL.

[fsaintjacques]

I'd like to chime in and say that this "just-works". I tried it locally and it works perfectly. I didn't do any stress test.

[seh]

@fsaintjacques, how did you integrate this branch into your project?

If we can update this to depend on OpenTelemetry version 0.17.0, I can start using it.

[XSAM]

Hey @seh, I upgraded the version of OTel to 0.17.0. Enjoy!

[seh]

Thank you, Sam. I was able to test it in my project by adding the following to my go.mod file:

replace go.opentelemetry.io/contrib/instrumentation/database/sql/otelsql => github.com/XSAM/opentelemetry-go-contrib/instrumentation/database/sql/otelsql v0.0.0-20210221151636-93de4d250415

I had written the branch name as "database-sql," but one of the go mod commands rewrote it with that computed reference.

[davidgwcurve]

hey! Is this PR anywhere close to getting merged? We would love to make use of this to get traces against our DBs!

[seh]

I'm still interested in being able to include query parameters in the spans, though I understand the security risk. We'd probably need to able to configure allowed or blocked sets of parameters, but that's fragile; parameters don't always have names that would allow identifying them.

Perhaps we could introduce something like database/sql.NamedArg (and its companion Named function that would indicate that the argument should be included as a field in the trace span. The driver wrapper would need to detect and unwrap such arguments to pass down to the base driver.

There's also the question of how we'd represent the sequence of arguments in a trace span. Would we write them in a single field, in the style of a JSON array? Would we create field names like "db.arg.1," "db.arg.2," and so on? Or maybe require that the hypothetical TracedArg type have a nonempty name, where we'd either use that name directly as the field name or a suffix to compose names like "db.arg.foo" for an argument designated as "foo." It would be difficult to ensure that such argument names would be unique.

[fsaintjacques]

I think this PR is big enough as it is. The proposed change would be better suited in a followup issue/PR. We're already 2 (+ committer) that voiced our desire to use this immediately.

[seh]

Yes, I too am using it today. I didn't intend to hold this up. I'm just sharing my impressions from having used it over the last week or so.

[nvx]

I mean if you're having to specially tag arguments anyway, might as well just add a tag to a parent span no?

[seh]

I'm already doing that. The utility here is for debugging when you pass the arguments in the wrong order, or when you're trying to figure out why a query behaves strangely with particular arguments. Now, you could say in those cases one should just inspect the parent spans to learn of those arguments, but that doesn't always provide the whole story—at least not easily. With Honeycomb, for example, it's not possible today to query for matching fields in related spans; your query predicate must be satisfied by a single span. Sometimes you're querying for things specific to the database, focusing on SQL statements, below the level of the application spans.

[davidgwcurve]

I think this PR is big enough as it is. The proposed change would be better suited in a followup issue/PR. We're already 2 (+ committer) that voiced our desire to use this immediately.

agreed, I would prefer to just see rapid iteration on this, add any additional features/fixes to go in new PRs. This has been open for 2 months now. If the tests are passing and if the people willing to consume the branch version of this are reporting it just works lets get it merged so the whole community can use it. Personally we have a need to get tracing for our databases and we're blocked whilst we wait for this to complete since we don't want to use prerelease versions.

[seh]

Thank you once again for the prompt update, Sam.

[vgarvardt]

Any progress on merging this?

@XSAM did a great job, but every time PR gets rebased and updated to the latest code base it expires (conflicts) in a week or so.

[Aneurysm9]

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.

@XSAM are you able to host this module independently for the time being?

[XSAM]

@Aneurysm9 Sure thing, I host this module in https://github.com/XSAM/otelsql.

[seh]

Thank you very much, Sam. I'm now using your other module path in concert with the version 0.19.0 release, and it works well.

[irl-segfault]

want to comment that this works great and would love to see it included in contrib.

[pellared]

db.statement will use this, which is a required attribute

db.statement is not "required". It is "conditional". Would be good to have an option to disable it. Still, the option can be added later in a separate PR.

[XSAM]

db.statement will use this, which is a required attribute

db.statement is not "required". It is "conditional". Would be good to have an option to disable it. Still, the option can be added later in a separate PR.

@pellared You can check this issue XSAM/otelsql#22. :)

[MrAlias]

I'm going to close this as the instrumentation is now being hosted at https://github.com/XSAM/otelsql in accordance with our new instrumentation policy.

[flc1125]

mark