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.SqlClient] Move package from main repository #1673

Merged
merged 11 commits into from
Apr 24, 2024

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Apr 22, 2024

Towards open-telemetry/opentelemetry-dotnet#5526

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@Kielek Kielek added the comp:instrumentation.sqlclient Things related to OpenTelemetry.Instrumentation.SqlClient label Apr 22, 2024
@Kielek
Copy link
Contributor Author

Kielek commented Apr 22, 2024

Open questions

  1. CHANGELOG.md - should it contain information about moving from main repository to the contib?
  2. What's about component owner file? Should it be unset as for other packages migrated from main repository or do you want to be assugned?

@open-telemetry/dotnet-approvers?

@Kielek Kielek force-pushed the migrate-sqlclientinstrumentation branch from ce07b4d to 1cc0734 Compare April 22, 2024 07:44
@Kielek Kielek force-pushed the migrate-sqlclientinstrumentation branch from 1cc0734 to 9fbfdf2 Compare April 22, 2024 07:53
@Kielek Kielek marked this pull request as ready for review April 22, 2024 08:09
@Kielek Kielek requested a review from a team April 22, 2024 08:09
@reyang
Copy link
Member

reyang commented Apr 22, 2024

  1. CHANGELOG.md - should it contain information about moving from main repository to the contib?

Probably not necessary IMHO. I think including the opentelemetry-dotnet PR link in the commit description is important though.

  1. What's about component owner file? Should it be unset as for other packages migrated from main repository or do you want to be assugned?

Not a blocker, leave it as empty for now, folks who have knowledge/experience + are willing to be the owner can claim it in separate PRs.

// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#pragma warning disable IDE0005 // Using directive is unnecessary.using System;
Copy link
Member

Choose a reason for hiding this comment

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

curious why do we need additional using? original file does not have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upstream - all usages have enable ImplicitUsings. In this repository, this file is also part of OpenTelemetry.Contrib.Tests.Shared. This feature is not enabled so I have to add it manually, but the SqlClient project enables Implicitusings - it requiers additional #pragma disable.

</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Options" Version="$(MicrosoftExtensionsOptionsPkgVer)" />
Copy link
Member

Choose a reason for hiding this comment

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

This is downgrading the version to 3.1.0?

Copy link
Contributor Author

@Kielek Kielek Apr 23, 2024

Choose a reason for hiding this comment

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

Good catch. Handled in 0cf5262.

We should consider unifying Microsoft.Extensions.Options version in this repository. Candidate for separate PR.

@Kielek Kielek merged commit e857280 into open-telemetry:main Apr 24, 2024
118 of 119 checks passed
@Kielek Kielek deleted the migrate-sqlclientinstrumentation branch April 24, 2024 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.sqlclient Things related to OpenTelemetry.Instrumentation.SqlClient
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants