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

Remove dependency from OpenTelemetry.Instrumentation.SqlClient #3059

Merged
merged 11 commits into from
Mar 22, 2024

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Mar 21, 2024

Used two a separate forlder for /Shared since I expect the other OTel libraries we need to include will need this too.

See #2819

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Mar 21, 2024
@eerhardt eerhardt requested a review from radical March 21, 2024 15:20
THIRD-PARTY-NOTICES.TXT Outdated Show resolved Hide resolved
@eerhardt
Copy link
Member

@reyang - Can you review the attribution to ensure we aren't violating the license here?

THIRD-PARTY-NOTICES.TXT Outdated Show resolved Hide resolved
@@ -0,0 +1,32 @@
# Vendoring code sync instructions
Copy link

Choose a reason for hiding this comment

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

@reyang
Copy link

reyang commented Mar 21, 2024

@reyang - Can you review the attribution to ensure we aren't violating the license here?

I've added couple suggestions, it looks good to me once these are addressed 👍

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

So we decided to go with building their code ourselves as opposed to re-disting their libraries inside our package?

This makes me nervous as we won't be running their tests in this code, nor would we be applying code fixes, and we don't really know how long are we going to be doing this.

Did we explore the option of just harvesting their library and including it in our package instead?

cc @eerhardt

src/Vendoring/README.md Outdated Show resolved Hide resolved
@eerhardt
Copy link
Member

So we decided to go with building their code ourselves as opposed to re-disting their libraries inside our package?

This makes me nervous as we won't be running their tests in this code, nor would we be applying code fixes, and we don't really know how long are we going to be doing this.

Did we explore the option of just harvesting their library and including it in our package instead?

cc @eerhardt

Redisting libraries in a nuget package isn't without drawbacks either. If the app brings in the OpenTelemetry.Instrumentation.SqlClient package through another dependency, there will be conflicts since 2 nuget packages brought in the same assembly. We would also need to sign it, etc, in order to redist it.

and we don't really know how long are we going to be doing this.

My understanding from @reyang is that the SqlClient library is targeted to ship stable later this calendar year. But I assume those plans can change. The other one we will need to give the same treatment is the StackExchange.Redis OTel library, which doesn't have a plan on going stable AFAIK.

<PackageReference Include="Microsoft.Extensions.Configuration.Binder" />
<PackageReference Include="Microsoft.Extensions.Diagnostics.HealthChecks" />
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" />
<PackageReference Include="OpenTelemetry.Instrumentation.AspNetCore" />
Copy link
Member

Choose a reason for hiding this comment

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

What's this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think when I imported some source this was required for a specific type. Now not anymore so I assume that I removed a file that was the culprit. Removed.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

dotnet_diagnostic.IDE1006.severity = silent

# IDE0028: Use collection initializers
dotnet_diagnostic.IDE0028.severity = silent
Copy link

Choose a reason for hiding this comment

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

@rajkumar-rangaraj @vishweshbankwar some of these settings seem to suggest that OpenTelemetry .NET repo is not following the best practices, would you follow up and see what needs to be updated to have these turned on at repo level? Thanks!

Copy link

Choose a reason for hiding this comment

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

@sebastienros sebastienros enabled auto-merge (squash) March 22, 2024 16:36
@sebastienros sebastienros merged commit 8ede877 into main Mar 22, 2024
8 checks passed
@sebastienros sebastienros deleted the sebros/sqlclient branch March 22, 2024 18:00
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants