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.

Sign up for GitHub

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

Jump to bottom

[Instrumentation.SqlClient] Update to follow new DB span conventions #2229

Merged
merged 10 commits into from
Oct 21, 2024
Merged

[Instrumentation.SqlClient] Update to follow new DB span conventions #2229

merged 10 commits into from
Oct 21, 2024

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Oct 18, 2024 •
edited
Loading

Fixes #2225

This PR is the first in a series of PRs. It introduces support for the OTEL_SEMCONV_STABILITY_OPT_IN environment variable to gate using the old or new conventions. It does not aim to fully adopt the new database conventions. It only updates conventions for the attributes currently reported by the instrumentation.

The documentation in the readme still only refers to the old attributes. I will update the readme in a different PR.

@alanwest alanwest requested a review from a team as a code owner October 18, 2024 22:49
@github-actions github-actions bot added the comp:instrumentation.sqlclient Things related to OpenTelemetry.Instrumentation.SqlClient label Oct 18, 2024
@alanwest alanwest changed the title Alanwest/new conventions sqlclient Update SqlClient instrumentation to follow new DB span conventions Oct 18, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.78%. Comparing base (71655ce) to head (4132fa6).
Report is 544 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2229      +/-   ##
==========================================
- Coverage   73.91%   71.78%   -2.14%     
==========================================
  Files         267      357      +90     
  Lines        9615    13779    +4164     
==========================================
+ Hits         7107     9891    +2784     
- Misses       2508     3888    +1380
Flag Coverage Δ
unittests-Contrib.Shared.Tests 83.17% <ø> (?)
unittests-Exporter.Geneva 58.60% <ø> (?)
unittests-Exporter.InfluxDB 95.88% <ø> (?)
unittests-Exporter.Instana 71.24% <ø> (?)
unittests-Exporter.OneCollector 94.34% <ø> (?)
unittests-Exporter.Stackdriver 75.73% <ø> (?)
unittests-Extensions 88.63% <ø> (?)
unittests-Extensions.Enrichment 100.00% <ø> (?)
unittests-Instrumentation.AWS 85.89% <ø> (?)
unittests-Instrumentation.AspNet 76.73% <ø> (?)
unittests-Instrumentation.AspNetCore 70.06% <ø> (?)
unittests-Instrumentation.ConfluentKafka 14.12% <ø> (?)
unittests-Instrumentation.ElasticsearchClient 79.87% <ø> (?)
unittests-Instrumentation.EntityFrameworkCore 57.06% <ø> (?)
unittests-Instrumentation.EventCounters 76.36% <ø> (?)
unittests-Instrumentation.GrpcNetClient 79.61% <ø> (?)
unittests-Instrumentation.Hangfire 93.58% <ø> (?)
unittests-Instrumentation.Http 73.57% <ø> (?)
unittests-Instrumentation.Owin 85.97% <ø> (?)
unittests-Instrumentation.Process 100.00% <ø> (?)
unittests-Instrumentation.Quartz 78.94% <ø> (?)
unittests-Instrumentation.Runtime 97.53% <ø> (?)
unittests-Instrumentation.SqlClient 91.26% <100.00%> (?)
unittests-Instrumentation.StackExchangeRedis 69.92% <ø> (?)
unittests-Instrumentation.Wcf 78.47% <ø> (?)
unittests-PersistentStorage 65.78% <ø> (?)
unittests-Resources.AWS 77.85% <ø> (?)
unittests-Resources.Azure 83.89% <ø> (?)
unittests-Resources.Container 72.41% <ø> (?)
unittests-Resources.Gcp 72.54% <ø> (?)
unittests-Resources.Host 73.94% <ø> (?)
unittests-Resources.OperatingSystem 77.20% <ø> (?)
unittests-Resources.Process 100.00% <ø> (?)
unittests-Resources.ProcessRuntime 77.08% <ø> (?)
unittests-Sampler.AWS 87.74% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ient/Implementation/SqlClientDiagnosticListener.cs 87.35% <100.00%> (ø)
...ent/Implementation/SqlEventSourceListener.netfx.cs 88.23% <100.00%> (ø)
....SqlClient/SqlClientTraceInstrumentationOptions.cs 98.79% <100.00%> (ø)

... and 374 files with indirect coverage changes

alanwest
alanwest commented Oct 19, 2024
View reviewed changes
src/OpenTelemetry.Instrumentation.SqlClient/SqlClientTraceInstrumentationOptions.cs
Comment on lines -258 to -261
if (!this.EnableConnectionLevelAttributes)
{
sqlActivity.SetTag(SemanticConventions.AttributePeerService, dataSource);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This instrumentation should not be emitting peer.service.

But just a reminder here... when peer.service was removed from HTTP client instrumentation it was unexpected see #1761.

Kielek
Kielek approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

In general LGTM.
Some todos/nits only.

src/OpenTelemetry.Instrumentation.SqlClient/Implementation/SqlClientDiagnosticListener.cs
@@ -115,15 +126,31 @@ public override void OnEventWritten(string name, object? payload)
case CommandType.StoredProcedure:
if (this.options.SetDbStatementForStoredProcedure)
Copy link
Contributor

Choose a reason for hiding this comment

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

Before making this package stable, we should reconsider this feature flag name.

same for SetDbStatementForText. It can be separate issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, all of the options will need to be reviewed before we ship stable. I lean toward only having one option for turning collection of db.query.text on/off. These options are also related to the sanitization I'll implement with #2221. Rather than just on/off, these options may need to be something like: off, on w/ sanitization, and on w/o sanitization.

src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md
Comment on lines +28 to +31
* **Breaking change**: The `peer.service` and `server.socket.address` attributes
are no longer emitted. Users should rely on the `server.address` attribute
for the same information. Note that `server.address` is only included when
the `EnableConnectionLevelAttributes` option is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Can it be done as separate, tiny PR? With information that it is updating network sem conv to exact version?
It will be easier to follow changes for end-users.

Copy link
Member Author

Choose a reason for hiding this comment

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

With information that it is updating network sem conv to exact version?

What do you mean? Are you saying the changelog should reference the version of semantic conventions where these changes were introduced?

@Kielek Kielek changed the title Update SqlClient instrumentation to follow new DB span conventions [Instrumentation.SqlClient] Update to follow new DB span conventions Oct 21, 2024
@alanwest alanwest merged commit 89fb052 into open-telemetry:main Oct 21, 2024
131 checks passed
@alanwest alanwest deleted the alanwest/new-conventions-sqlclient branch October 21, 2024 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kielek Kielek Kielek approved these changes

Assignees
No one assigned
Labels
comp:instrumentation.sqlclient Things related to OpenTelemetry.Instrumentation.SqlClient
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[sql] Introduce OTEL_SEMCONV_STABILITY_OPT_IN to gate new DB conventions
2 participants
@alanwest @Kielek