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

Add support for System.Data.SqlClient (Nuget package) #3058

Merged
merged 14 commits into from
Oct 31, 2023
Merged

Add support for System.Data.SqlClient (Nuget package) #3058

merged 14 commits into from
Oct 31, 2023

Conversation

RassK
Copy link
Contributor

@RassK RassK commented Oct 31, 2023 •
edited
Loading

Why

Fixes #3047

What

  • Adds System.Data.SqlClient (Nuget package) traces instrumentation from 4.8.5 (every earlier version has security vulnerabilities).

Tests

  • Added new TestApplication for System.Data.SqlClient
    (Currently chose copy-paste over pre-processor directives matrix, in case of better idea, let me know!)

Checklist

  • CHANGELOG.md is updated.
  • Documentation is updated.
  • New features are covered by tests.

@RassK RassK requested a review from a team October 31, 2023 08:52
pellared
pellared reviewed Oct 31, 2023
View reviewed changes
Copy link
Member

@pellared pellared left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good overall. We just need to better document the supported versions in config.md.

test/test-applications/integrations/TestApplication.SqlClient.System.NetFramework/Program.cs Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
test/test-applications/integrations/TestApplication.SqlClient.System/Program.cs Outdated Show resolved Hide resolved
RassK and others added 3 commits October 31, 2023 12:49
@RassK @pellared
Update docs/config.md
185bed7 
Co-authored-by: Robert Pająk <[email protected]>
@RassK @pellared
Update test/test-applications/integrations/TestApplication.SqlClient.…
04e6379 
…System.NetFramework/Program.cs

Co-authored-by: Robert Pająk <[email protected]>
@RassK @pellared
Update test/test-applications/integrations/TestApplication.SqlClient.…
0e1d8ab 
…System/Program.cs

Co-authored-by: Robert Pająk <[email protected]>
@RassK
Copy link
Contributor Author

RassK commented Oct 31, 2023

Looks good overall. We just need to better document the supported versions in config.md.

Maybe just to break that 1 row into 3 with the same key? Could be much more readable and we can use different versions there.

@pellared
Copy link
Member

pellared commented Oct 31, 2023 •
edited
Loading

Looks good overall. We just need to better document the supported versions in config.md.

Maybe just to break that 1 row into 3 with the same key? Could be much more readable and we can use different versions there.

Or maybe we should have 3 different keys?

EDIT: For this PR I think we can just add more details in the footer note.

@RassK
Copy link
Contributor Author

RassK commented Oct 31, 2023

Or maybe we should have 3 different keys?

Better not, it's the same initializer. It will work anyway with all 3 options.

pjanotti
pjanotti approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

LGTM

Kielek
Kielek reviewed Oct 31, 2023
View reviewed changes
tools/LibraryVersionsGenerator/PackageVersionDefinitions.cs Show resolved Hide resolved
CHANGELOG.md
@@ -9,6 +9,9 @@ This component adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.h

### Added

- Added support for [System.Data.SqlClient](https://www.nuget.org/packages/System.Data.SqlClient/)
(NuGet package) traces instrumentation from `4.8.5`.
Copy link
Contributor

Choose a reason for hiding this comment

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

if we support only 4.8.5+ this should be documented in config.md. Current version seems to support all available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3058 (comment)
vs
#3058 (comment)

any preferences?

Copy link
Contributor

Choose a reason for hiding this comment

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

Footer note

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then footnote it is - 3a570e2

@Kielek Kielek merged commit 11d051a into open-telemetry:main Oct 31, 2023
28 checks passed
@RassK RassK deleted the system-data-sqlclient-inst branch October 31, 2023 21:13
@Kielek Kielek mentioned this pull request Nov 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kielek Kielek Kielek left review comments

@pellared pellared pellared left review comments

@nrcventura nrcventura nrcventura approved these changes

@zacharycmontoya zacharycmontoya zacharycmontoya approved these changes

@rajkumar-rangaraj rajkumar-rangaraj rajkumar-rangaraj approved these changes

@pjanotti pjanotti pjanotti approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automatic instrumentation of System.Data.SqlClient not working.
7 participants
@RassK @pellared @pjanotti @Kielek @rajkumar-rangaraj @zacharycmontoya @nrcventura