Skip to content

contrib/database/sql: only record spans for implemented driver methods#336

Merged
gbbr merged 2 commits into
v1from
gbbr/sqlx-errskip
Oct 1, 2018
Merged

contrib/database/sql: only record spans for implemented driver methods#336
gbbr merged 2 commits into
v1from
gbbr/sqlx-errskip

Conversation

@gbbr

@gbbr gbbr commented Sep 28, 2018

Copy link
Copy Markdown
Contributor

Instrumentation for some packages (such as jmoiron/sqlx) is tracing calls returning driver.ErrSkip, causing not only duplicate traces, but also APM errors where in fact there were none.

From (database/sql/driver).ErrSkip:

// ErrSkip may be returned by some optional interfaces' methods to
// indicate at runtime that the fast path is unavailable and the sql
// package should continue as if the optional interface was not
// implemented. ErrSkip is only supported where explicitly
// documented.
var ErrSkip = errors.New("driver: skip fast-path; continue as if unimplemented")

This is not a user error and in these cases we should not perform any tracing. See #270 for a detailed overview of the problem.

Fixes #270

TODO

  • Test with Datadog app.
  • Try to write a unit test.

@gbbr gbbr added bug unintended behavior that has to be fixed apm:ecosystem contrib/* related feature requests or bugs do-not-merge/WIP labels Sep 28, 2018
@gbbr gbbr added this to the 1.4.0 milestone Sep 28, 2018
@gbbr
gbbr force-pushed the gbbr/sqlx-errskip branch from 06fbf6d to e3c28f7 Compare September 28, 2018 10:15
@gbbr
gbbr force-pushed the gbbr/sqlx-errskip branch from e3c28f7 to 336bf6f Compare September 28, 2018 10:16
@gbbr
gbbr requested review from dd-caleb and palazzem September 28, 2018 16:51
@gbbr

gbbr commented Sep 28, 2018

Copy link
Copy Markdown
Contributor Author

@dd-caleb, @palazzem I have asked for your reviews because it seems like a sensitive change.

@dd-caleb dd-caleb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@gbbr
gbbr merged commit cbae612 into v1 Oct 1, 2018
@gbbr
gbbr deleted the gbbr/sqlx-errskip branch October 1, 2018 15:47
mingrammer pushed a commit to mingrammer/dd-trace-go that referenced this pull request Dec 22, 2020
DataDog#336)

* contrib/database/sql: only record spans for implemented driver methods

* contrib/mongodb/mongo-go-driver: update to latest release
nsrip-dd added a commit that referenced this pull request Jun 21, 2023
…tion trace tasks

After #336, spans for database operations are created *after* the
operation. This is necessary because some operations may return ErrSkip
if they aren't supported, and creating spans with those errors is very
noisy. But because of this, execution trace tasks associated with those
spans would appear to be only a few microseconds long since they are
created alongside the span and thus only cover the very end of the
operation.

This commit creates execution trace tasks (if the execution tracer is
enabled) before the operations, and provides a way of communicating to
the APM tracer that it doesn't need to create a task for the span. The
APM tracer will still annotate the task with the span ID even if a task
was already created.

Still TODO: add tests
nsrip-dd added a commit that referenced this pull request Jun 21, 2023
…tion trace tasks

After #336, spans for database operations are created *after* the
operation. This is necessary because some operations may return ErrSkip
if they aren't supported, and creating spans with those errors is very
noisy. But because of this, execution trace tasks associated with those
spans would appear to be only a few microseconds long since they are
created alongside the span and thus only cover the very end of the
operation.

This commit creates execution trace tasks (if the execution tracer is
enabled) before the operations, and provides a way of communicating to
the APM tracer that it doesn't need to create a task for the span. The
APM tracer will still annotate the task with the span ID even if a task
was already created.

Still TODO: add tests
nsrip-dd added a commit that referenced this pull request Jun 21, 2023
…tion trace tasks

After #336, spans for database operations are created *after* the
operation. This is necessary because some operations may return ErrSkip
if they aren't supported, and creating spans with those errors is very
noisy. But because of this, execution trace tasks associated with those
spans would appear to be only a few microseconds long since they are
created alongside the span and thus only cover the very end of the
operation.

This commit creates execution trace tasks (if the execution tracer is
enabled) before the operations, and provides a way of communicating to
the APM tracer that it doesn't need to create a task for the span. The
APM tracer will still annotate the task with the span ID even if a task
was already created.

Still TODO: add tests
nsrip-dd added a commit that referenced this pull request Jun 26, 2023
…trace tasks (#2060)

After #336, spans for database operations are created after the operation.
This is necessary because some operations may return ErrSkip if they aren't
supported, and creating spans with those errors is very noisy. But because of
this, execution trace tasks associated with those spans would appear to be only
a few microseconds long since they are created alongside the span and thus only
cover the very end of the operation.

This commit creates execution trace tasks (if the execution tracer is enabled)
before the operations, and provides a way of communicating to the APM tracer
that it doesn't need to create a task for the span. The APM tracer will still
annotate the task with the span ID even if a task was already created.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

apm:ecosystem contrib/* related feature requests or bugs bug unintended behavior that has to be fixed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants