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

MongoDB - instrumenation for .NET Framework #2390

Merged
merged 13 commits into from
Apr 14, 2023

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Mar 28, 2023

Why

Fixes #2328

What

  • MongoDB - instrumentation for .NET Framework
  • Move instrumentation library dependency from Addition Dependencies to main project.

Tests

Executed locally on Docker for Windows with Linux.
Testocontainers for now does not support Docker for Windows with Windows. No possibility to enable it in pipeline: testcontainers/moby-ryuk#40 (comment)

Checklist

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

@Kielek Kielek requested a review from a team March 28, 2023 11:13
@github-actions github-actions bot requested a review from theletterf March 28, 2023 11:14
@Kielek
Copy link
Contributor Author

Kielek commented Mar 28, 2023

Alternative approach discussed with @RassK - manually copy MongoDB instrumentation library instead of removing non-needed libraries from the output folder.

As the goal, for now, is to make nuget scenario working (+ .NET Framework support) I would keep current scenario. Proposal from Rasmus can be handled later as future optimisations.

@RassK
Copy link
Contributor

RassK commented Mar 28, 2023

Alternative approach discussed with @RassK - manually copy MongoDB instrumentation library instead of removing non-needed libraries from the output folder.

As the goal, for now, is to make nuget scenario working (+ .NET Framework support) I would keep current scenario. Proposal from Rasmus can be handled later as future optimisations.

This will also halt the huge progress made in #2294. Meanwhile @pjanotti's suggestion for Nuget is also easier to achieve.

@Kielek
Copy link
Contributor Author

Kielek commented Mar 28, 2023

@RassK, I think that #2294 can be adapted also to direct references approach. Now, ExcludedAssets are generated manually.

@RassK
Copy link
Contributor

RassK commented Mar 28, 2023

@RassK, I think that #2294 can be adapted also to direct references approach. Now, ExcludedAssets are generated manually.

No, it's only addressed for additional deps / shared store purpose.
(designed for loosely coupled dependencies, instead of direct dependencies)

@Kielek
Copy link
Contributor Author

Kielek commented Mar 28, 2023

Potential follow up commit with nuget support: Kielek@4cf165e

@pjanotti
Copy link
Contributor

@Kielek @RassK this PR is an interesting possible path forward for the "usual" distribution (ie.: non-Nuget). That said I think we have to step back and design the overall solution.

Regarding:

Testocontainers for now does not support Docker for Windows with Windows. No possibility to enable it in pipeline: testcontainers/moby-ryuk#40 (comment)

I thought the issue was that GH Windows runners can only run Windows containers. The ASP.NET test launches a Windows container without the ryuk stuff. That said it is true that ryuk launches a Linux container. If we want to test MongoDB instrumentation on .NET Framework it will be necessary to run MongoDB in a Windows container.

@RassK
Copy link
Contributor

RassK commented Mar 28, 2023

@Kielek @RassK this PR is an interesting possible path forward for the "usual" distribution (ie.: non-Nuget). That said I think we have to step back and design the overall solution.

For me it seems to be in any way step backwards.

  • It's a complex package (many dependencies + native dependencies)
  • It has dependencies not connected to MongoDB (DnsClient and SharpCompress)
  • I think we should be more dynamic and adaptive to the application, than more static and ship a relatively large package.
  • Being static narrows down the supported version range. Currently it's theoretically possible to extend the support by having multiple versions of the adapter module and then dynamically loading the correct version.

Copy link
Member

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

@Kielek LMK when this goes downstream—thx!

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 w/ a minor suggestion.

@@ -21,4 +21,29 @@
<!-- Microsoft.Bcl.AsyncInterfaces is required by StackExchange.Redis. ExcludeAssets="all" prevents copying it to the output -->
<PackageReference Include="Microsoft.Bcl.AsyncInterfaces" ExcludeAssets="all" />
</ItemGroup>

<ItemGroup>
<!-- DnsClient is required by MongoDB.Driver.Core.Extensions.DiagnosticSources. ExcludeAssets="all" prevents copying it to the output -->
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid the verbosity and the copy/paste mistake it seems better to have a single comment before the ItemGroup. Something like "Transient dependencies from MongoDB.Driver.Core.Extensions.DiagnosticSources that are shipped with the applications. ExcludeAssets="all" prevents copying them to the output"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted common comment. b7ca218
I would keep explanation per library - I expect more to come. Part of them probably can be linked by more than one library.

@Kielek Kielek enabled auto-merge (squash) April 14, 2023 06:27
@Kielek Kielek merged commit 79d38c1 into open-telemetry:main Apr 14, 2023
@Kielek Kielek deleted the mongodb-netfx-support branch April 14, 2023 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NuGet package missing assemblies for MongoDB instrumentation
4 participants