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

Fix dotnet CLI usage: additional deps support to roll-forward #2165

Merged
merged 8 commits into from
Feb 9, 2023

Conversation

pjanotti
Copy link
Contributor

@pjanotti pjanotti commented Feb 3, 2023

Why

To fix dotnet CLI usage.

Fixes #1744

What

  • Modify the shared store so it allows framework roll forward as configured for Roslyn, details here. This fixes the issues with dotnet CLI

Tests

  • Add integration tests for typical dotnet CLI usage.

Checklist

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

@pjanotti pjanotti requested a review from a team February 3, 2023 19:10
@pellared
Copy link
Member

pellared commented Feb 6, 2023

dotnet publish -f net7.0 -c Release ./test/test-applications/integrations/TestApplication.Smoke
export OTEL_DOTNET_AUTO_HOME="${PWD}/bin/tracer-home"
. ./instrument.sh
./test/test-applications/integrations/TestApplication.Smoke/bin/Release/net7.0/publish/TestApplication.Smoke

We could change it (and one more place) to

          export OTEL_DOTNET_AUTO_HOME="${PWD}/bin/tracer-home"
          . ./instrument.sh
          cd test/test-applications/integrations/TestApplication.Smoke
          dotnet run -f net7.0

I think it was the case before we bumped to .NET 7....

@pjanotti
Copy link
Contributor Author

pjanotti commented Feb 7, 2023

This is kind of ironic but hits the problem with the VBCSCompiler as intended, see https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/actions/runs/4110389057/jobs/7093155343#step:5:4366 and here for the explanation.

@pellared
Copy link
Member

pellared commented Feb 7, 2023

Regarding my prev. comment... I have not noticed that it is a PR 🤦

@pjanotti
Copy link
Contributor Author

pjanotti commented Feb 8, 2023

Adding the missing probe path while building the shared store, per this comment, fixed the issue locally.

@pjanotti
Copy link
Contributor Author

pjanotti commented Feb 8, 2023

Oh yeah forgot to update the test with the images...

@rajkumar-rangaraj
Copy link
Contributor

Should we keep both current and roll-forward version?
I think removing the current version should not cause an issue.

Current
/store/x64/net6.0/microsoft.extensions.configuration.abstractions/7.0.0/lib/net6.0/Microsoft.Extensions.Configuration.Abstractions.dll

Roll-Forward
/store/x64/net6.0/microsoft.extensions.configuration.abstractions/7.0.0/lib/net7.0/Microsoft.Extensions.Configuration.Abstractions.dll

@pjanotti pjanotti force-pushed the add-dotnet-cli-tests branch from c1d900f to f56fac0 Compare February 9, 2023 02:20
@github-actions github-actions bot requested a review from theletterf February 9, 2023 02:20
@pjanotti
Copy link
Contributor Author

pjanotti commented Feb 9, 2023

Should we keep both current and roll-forward version?

That's a good point. I did a quick test and as long as the additional deps.json is correctly updated I didn't see any issues instrumenting net6.0 applications even without explicitly setting roll-forward. However, I couldn't quickly get the additional deps file to be generated correctly (it needs to have lib/net7.0/ instead of lib/net6.0/). I will open a separate issue to handle that.

@pjanotti pjanotti changed the title Add dotnet CLI integration tests Fix dotnet CLI usage: additional deps support to roll-forward Feb 9, 2023
@pjanotti
Copy link
Contributor Author

pjanotti commented Feb 9, 2023

@otel-telemetry/dotnet-instrumentation-approvers

build/Build.Steps.cs Outdated Show resolved Hide resolved
process.ExitCode.Should().Be(0, "Test application exited with non-zero exit code");
}

private void RunAppWithDotNetCliAndAssertHttpSpans(string arguments)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private void RunAppWithDotNetCliAndAssertHttpSpans(string arguments)
private void TestDotNetCli(string arguments)

IMO it would be more clear 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum, this one I like the long name: we are "testing" every execution of dotnet CLI the one with the long method name is different in that we want to run the actual application and check for the HTTP spans.

test/IntegrationTests/DotNetCliTests.cs Outdated Show resolved Hide resolved
test/IntegrationTests/DotNetCliTests.cs Outdated Show resolved Hide resolved
test/IntegrationTests/DotNetCliTests.cs Outdated Show resolved Hide resolved
test/IntegrationTests/DotNetCliTests.cs Outdated Show resolved Hide resolved
test/IntegrationTests/DotNetCliTests.cs Outdated Show resolved Hide resolved
@pjanotti pjanotti force-pushed the add-dotnet-cli-tests branch from 5dd088e to b599fa4 Compare February 9, 2023 17:21
@pjanotti pjanotti enabled auto-merge (squash) February 9, 2023 17:38
@pjanotti pjanotti merged commit 7b59b25 into open-telemetry:main Feb 9, 2023
@pjanotti pjanotti deleted the add-dotnet-cli-tests branch February 9, 2023 19:07
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.

Instrumentation does not work with dotnet CLI
5 participants