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

[Instrumentation.EntityFrameworkCore] Update semantic conventions for stable release #2130

Merged
merged 13 commits into from
Oct 17, 2024

Conversation

joegoldman2
Copy link
Contributor

@joegoldman2 joegoldman2 commented Oct 2, 2024

Changes

Add support for OTEL_SEMCONV_STABILITY_OPT_IN following https://github.com/open-telemetry/semantic-conventions/blob/main/docs/non-normative/db-migration.md.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@joegoldman2 joegoldman2 requested a review from a team as a code owner October 2, 2024 15:30
@github-actions github-actions bot added the comp:instrumentation.entityframeworkcore Things related to OpenTelemetry.Instrumentation.EntityFrameworkCore label Oct 2, 2024
Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 71.73%. Comparing base (71655ce) to head (46f1e98).
Report is 540 commits behind head on main.

Files with missing lines Patch % Lines
...mplementation/EntityFrameworkDiagnosticListener.cs 30.76% 9 Missing ⚠️
src/Shared/DatabaseSemanticConventionHelper.cs 83.33% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2130      +/-   ##
==========================================
- Coverage   73.91%   71.73%   -2.18%     
==========================================
  Files         267      357      +90     
  Lines        9615    13756    +4141     
==========================================
+ Hits         7107     9868    +2761     
- Misses       2508     3888    +1380     
Flag Coverage Δ
unittests-Contrib.Shared.Tests 83.17% <83.33%> (?)
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% <59.09%> (?)
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 90.55% <ø> (?)
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 Δ
...eworkCore/EntityFrameworkInstrumentationOptions.cs 100.00% <100.00%> (ø)
src/Shared/DatabaseSemanticConventionHelper.cs 83.33% <83.33%> (ø)
...mplementation/EntityFrameworkDiagnosticListener.cs 53.03% <30.76%> (+0.64%) ⬆️

... and 374 files with indirect coverage changes

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 10, 2024
@github-actions github-actions bot removed the Stale label Oct 11, 2024
@Kielek
Copy link
Contributor

Kielek commented Oct 14, 2024

@joegoldman2, if you want to proceed with this PR, please consider https://github.com/open-telemetry/semantic-conventions/blob/main/docs/non-normative/db-migration.md

There were semantic convention helper for HTTP in main repository. It can be easily adjusted: https://github.com/open-telemetry/opentelemetry-dotnet/pull/5270/files#diff-c94fc23f1212ef9513764dd888ec8c98d2d59b2d51a0aabb311fa0652ec8b059

Keep in mind that network related attributes can be fixed now, the problem is only with SQL releated part.

@joegoldman2 joegoldman2 requested a review from Kielek October 14, 2024 10:22
@joegoldman2 joegoldman2 changed the title [Instrumentation.EntityFrameworkCore] Update few attributes to align with latest Semantic Conventions [Instrumentation.EntityFrameworkCore] Update semantic conventions for stable release Oct 14, 2024
@joegoldman2
Copy link
Contributor Author

I tested the change manually on a test app, it is working.


if (this.options.EmitNewAttributes)
{
activity.AddTag(AttributeDbNamespace, database);
Copy link
Member

@alanwest alanwest Oct 16, 2024

Choose a reason for hiding this comment

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

@trask @lmolkova what are your thoughts here?

Per footnote 4 in the conventions:

Semantic conventions for individual database systems SHOULD document what db.namespace means in the context of that system.

Since this instrumentation is for an ORM and supports many database systems, this would ideally respect the conventions declared for the target database system though I don't know if that will be possible.

Also not all systems this instrumentation supports have documentation for what db.namespace means.

Thoughts on ORMs in general?

Copy link
Member

Choose a reason for hiding this comment

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

we have a similar issue in Java (https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/jdbc/library/src/test/java/io/opentelemetry/instrumentation/jdbc/internal/JdbcConnectionUrlParserTest.java)

technically, I think stable JDBC instrumentation should only emit stable instrumentation by default, which means only emitting instrumentation for stable db.system values

in practice, this would mean that we can't mark the JDBC instrumentation as stable even after we update it to emit the stable database semconv (unless we want to force lots of users to opt-in to the unstable db.system values, which I don't think is a great experience)

I'm hoping this will motivate us to push semconv definitions for db.namespace for all of these other databases quickly, we just haven't had the bandwidth to do it yet and didn't want to block initial database semconv stability on it

Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

This PR looks good to me. There's still things left to do, but I think this is a step forward, so approving/merging.

I will open issues, but here's a quick summary off hand for things left to do:

  1. Collecting additional attributes
    • error.type / db.response.status_code
    • Adding option to parse db.collection.name / db.operation.name (this is something I'm working on in the context of the SQL instrumentation and should also be applicable here)
  2. Adding option to sanitize db.query.text (also working on this for SQL instrumentation)
  3. Defining our requirements for declaring this instrumentation stable. It's an ORM, so it may have some unique things to think through see [Instrumentation.EntityFrameworkCore] Update semantic conventions for stable release #2130 (comment)

@alanwest alanwest merged commit 5a9c9a8 into open-telemetry:main Oct 17, 2024
127 of 128 checks passed
@joegoldman2 joegoldman2 deleted the ef-latest-semantic branch October 18, 2024 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.entityframeworkcore Things related to OpenTelemetry.Instrumentation.EntityFrameworkCore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants