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

Add instrumentation for the database/sql package #88

Merged
merged 52 commits into from
Sep 20, 2021

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Sep 8, 2021

This new module instruments the built-indatabase/sql package. The instrumentation wraps all calls made with this package and is designed with a similar registration method for instrumentation setup (more on that bellow).

  • Comply with OpenTelemetry database semantic conventions
  • Package documentation
  • Exported objects are documented
  • Tests and linting are passing
  • Includes dependabot config
  • Library added to README.md

Instrumentation of specific drivers

The design of the splunksql package is such that users wanting an instrumented version of their driver and the database/sql package need to do two things.

  1. Update their import of the driver they are using to the instrumented driver.
  2. Use the splunksql.Open function instead of the sql.Open function.

To understand this flow, take for instance the generic example of a user using the MySQL driver with the database/sql package. To start their code looks like this.

import (
	"database/sql"

	_ "github.com/go-sql-driver/mysql"
)

// ...

db, err := sql.Open("mysql", "user:password@/dbname")
// ...

To use this instrumentation, their code will need to be updated to this.

import (
	"database/sql"

	_ "github.com/signalfx/splunk-otel-go/instrumentation/github.com/go-sql-driver/mysql/splunkmysql"
	"github.com/signalfx/splunk-otel-go/instrumentation/database/sql/splunksql"
)

// ...

db, err := splunksql.Open("mysql", "user:password@/dbname")
// ...

The import of github.com/signalfx/splunk-otel-go/instrumentation/github.com/go-sql-driver/mysql/splunkmysql will import the instrumented github.com/go-sql-driver/mysql (registering its driver with the database/sql package) and itself register instrumentation specific setup with the splunksql package (e.g. this). Then when the user calls splunksql.Open (which is a drop in replacement for sql.Open based on its variadic Option design). the registered driver and instrumentation setup will be loaded appropriately. From this, telemetry that knows what database it is for and annotated according to the OpenTelemetry semantic conventions is produced.

The need to register instrumentation setup comes from the need to parse the database connection string. That string will contain needed annotations and the way it is parsed is going to be specific to the database being instrumented.

This registration method differed from the existing signalfx-go-tracing instrumentation. There the database/sql package is designed to hold all parsing logic. This is a design that will not extend well, unlike the one presented here.

Uninstrumented drivers

If the user does not import an instrumentation package for the driver they are using, or one does not exist, the splunksql.Open function can still be extended to accommodate this. The returned sql.DB will still use an instrumented Driver which will produce telemetry. If the user passes attributes conforming to OTel semantic conventions using the WithAttributes Option as a parameter the produced telemetry will comply with the OTel specification.

MrAlias and others added 30 commits September 1, 2021 08:16
Have instrumentation register setup configuration for the driver they
are instrumenting. This moves the internal dsn parsing to the
instrumentation itself.
@pellared
Copy link
Contributor

pellared commented Sep 13, 2021

Do you want to add a splunksql/instrumentation/database/sql/splunksql/README.md in this PR or in another one? Moreover, I think it would be nice to add an example_test.go - I think it should demonstrate the "Uninstrumented drivers" scenario as examples for the instrumented ones could be in their respective directories.

Also, we need to think if we should not move this PR to https://github.com/open-telemetry/opentelemetry-go-contrib

Can this approach of instrumenting ng via otelsql.Open can live side-by-side with the approach of instrumenting via otelsql.Register proposed here
open-telemetry/opentelemetry-go-contrib#505?

@MrAlias
Copy link
Contributor Author

MrAlias commented Sep 13, 2021

Do you want to add a splunksql/instrumentation/database/sql/splunksql/README.md in this PR or in another one? Moreover, I think it would be nice to add an example_test.go - I think it should demonstrate the "Uninstrumented drivers" scenario as examples for the instrumented ones could be in their respective directories.

Yeah, let's do that in a follow on PR. This is quite large already.

@MrAlias MrAlias mentioned this pull request Sep 13, 2021
6 tasks
README.md Show resolved Hide resolved
@pellared
Copy link
Contributor

An idea for the next PR with documentation.

I think it may be a good idea to demonstrate how to add an additional attribute like db.sql.table. Never done it but I believe it is possible to it via ctx.

Copy link
Contributor

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I guess your are finishing adding explanations to places with nolint: 😉

@MrAlias MrAlias merged commit 399ffb9 into signalfx:main Sep 20, 2021
@MrAlias MrAlias deleted the splunksql branch September 20, 2021 19:44
@MrAlias MrAlias mentioned this pull request Jan 12, 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.

3 participants