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 .NET 9 in tests for AspNetCore, Runtime and Process instrumentation libraries #2313

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

Add .NET 9 in tests for AspNetCore, Runtime and Process instrumentation libraries #2313

wants to merge 25 commits into from

Conversation

xiang17
Copy link
Contributor

@xiang17 xiang17 commented Nov 12, 2024 •
edited
Loading

Depends on and merged commits from #2316. Please review that PR first.

Towards #2319.

Changes

Add .NET 9 target framework in tests for AspNetCore, Runtime and Process instrumentation libraries.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • [ ] Appropriate CHANGELOG.md files updated for non-trivial changes: only updated tests.
  • [ ] Changes in public API reviewed (if applicable)

@github-actions github-actions bot added infra Infra work - CI/CD, code coverage, linters comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore comp:instrumentation.process Things related to OpenTelemetry.Instrumentation.Process comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime labels Nov 12, 2024
@codecov Codecov
Copy link

codecov bot commented Nov 12, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.02%. Comparing base (71655ce) to head (ecef58a).
Report is 595 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2313      +/-   ##
==========================================
- Coverage   73.91%   72.02%   -1.90%     
==========================================
  Files         267      359      +92     
  Lines        9615    13761    +4146     
==========================================
+ Hits         7107     9911    +2804     
- Misses       2508     3850    +1342
Flag Coverage Δ
unittests-Exporter.InfluxDB 93.92% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 379 files with indirect coverage changes

@xiang17 xiang17 mentioned this pull request Nov 12, 2024
Merged
1 task
@xiang17
The last CI run failed with `Unable to find package Microsoft.AspNetC…
927b18a 
…ore.App.Ref with version (= 8.0.11)`. However 8.0.11 was released less than 20 min ago. Maybe the restore step had a delay and didn't get the new version yet?

D:\a\opentelemetry-dotnet-contrib\opentelemetry-dotnet-contrib\src\OpenTelemetry.Exporter.Geneva\OpenTelemetry.Exporter.Geneva.csproj : error NU1102: Unable to find package Microsoft.AspNetCore.App.Ref with version (= 8.0.11)
D:\a\opentelemetry-dotnet-contrib\opentelemetry-dotnet-contrib\src\OpenTelemetry.Exporter.Geneva\OpenTelemetry.Exporter.Geneva.csproj : error NU1102:   - Found 128 version(s) in NuGet [ Nearest version: 9.0.0-preview.1.24081.5 ]
D:\a\opentelemetry-dotnet-contrib\opentelemetry-dotnet-contrib\src\OpenTelemetry.Exporter.Geneva\OpenTelemetry.Exporter.Geneva.csproj : error NU1102:   - Found 0 version(s) in C:\Program Files\dotnet\library-packs
  Failed to restore D:\a\opentelemetry-dotnet-contrib\opentelemetry-dotnet-contrib\src\OpenTelemetry.Exporter.Geneva\OpenTelemetry.Exporter.Geneva.csproj (in 20.62 sec).
@xiang17
ubuntu-latest is pointing to a newer version than ubuntu-24.04 now. b…
ca18ad3 
…uild-test-exporter-geneva-integration / build-test (ubuntu-24.04, net8.0) runs on Ubuntu 22.04.1 LTS. If not set, the version is newer. For example, build-test-exporter-influxdb / build-test (ubuntu-latest, net8.0) runs on Ubuntu 22.04.5 LTS.
@xiang17
Upgrade Kafka and StackExchangeRedis integration tests which uses .NE…
ba5056f 
…T SDK in a docker.
@xiang17
Update .NET SDK version for AOT Compatibility Test
5d7f503
@xiang17
Install .NET 9 for AOT tests. The .NET SDK 9 is not installed by defa…
e034be3 
…ult yet
@xiang17
Update docker invokation in CI to have both .NET 8 and 9
30341a1
@xiang17
In docker-compose.net8.0.yml, setting BUILD_SDK_VERSION to 9.0 would …
c723ee1 
…fail in GitHub CI for now, both for confluentkafka-integration and stackexchangeredis-integration. The current latest SDK in mcr.microsoft.com/dotnet/sdk:9.0 is 9.0.100-rc.2.24474.11, as shown in CI.

I'll revert global.json to use rc.2 for now.
@xiang17
The tests are failing after upgrading to .NET 9. Disabling them for n…
cf4cef8 
…ow to be inspected later.
@xiang17
remove unused using statement
c875d77
@xiang17
Removing .net 9 docker tests because the kafka and StackExchangeRedis…
3c7638e 
… doesn't have net9.0 targets yet. I'll leave to the component owners to upgrade them.
@xiang17
Amend last commit: update CI to not invoke .NET 9 for docker integ tests
de143fa
@xiang17
Add .NET 9 in target framework for AOT compatibility test
0f36251
@xiang17
Merge branch 'main' into xiang17/dotnet9Tests
0d5bcda
@xiang17
Merge branch 'xiang17/dotnet9CIExperiment' into xiang17/dotnet9Tests
0e6cf78 
# Conflicts:
#	build/Common.nonprod.props
@xiang17
Add net9.0 for AspNetCore test. This needs to be individually confi…
a702848 
…gured because the default list includes net462
Kielek
Kielek reviewed Nov 13, 2024
View reviewed changes
build/Common.nonprod.props Outdated
Comment on lines 33 to 34
<SupportedNetTargetsWithNet9>net9.0;net8.0</SupportedNetTargetsWithNet9>
<SupportedNetTargets>net8.0</SupportedNetTargets>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just try to add to all packages by default? If it pass without issues, we could merge it. IMO it should be standard procedure on the repo-level instead of handling it one by one.

The only exception can occur when the tests are failing. Then we can create SupportedNetTargetsWithoutNet9 to handle is separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were a lot of issues in this PR and I already split into another PR (#2316).

Can I keep this PR simple and do the follow up for other packages in another PR?

@xiang17
Merge branch 'main' into xiang17/dotnet9Tests
10d5f58 
# Conflicts:
#	.github/workflows/verifyaotcompat.yml
#	test/OpenTelemetry.AotCompatibility.TestApp/OpenTelemetry.AotCompatibility.TestApp.csproj
@github-actions github-actions bot removed comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis comp:instrumentation.confluentkafka Things related to OpenTelemetry.Instrumentation.ConfluentKafka labels Nov 13, 2024
@rajkumar-rangaraj
Copy link
Contributor

Could you please update the changelog's to state .NET 9.0 was added.

@xiang17
Copy link
Contributor Author

xiang17 commented Nov 13, 2024

Could you please update the changelog's to state .NET 9.0 was added.

I wanted to but realized all CHANGELOG.md files are in src folder. And this PR only happens for test projects (all under test folder except one in build/Common.nonprod.props) and in CI.

I could still add an entry in all involved projects but technically they have no changes (because their target framework is NetMinimumSupportedVersion instead of a list of all supported versions).

@rajkumar-rangaraj
Copy link
Contributor

I wanted to but realized all CHANGELOG.md files are in src folder.

I think the plan is to add net9.0 targets to AspNetCore, Runtime and Process instrumentation libraries projects. Are you planning to cover in new PR?

@github-actions github-actions bot added documentation Improvements or additions to documentation comp:instrumentation.grpccore Things related to OpenTelemetry.Instrumentation.GrpcCore labels Nov 14, 2024
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

@twenzel twenzel Awaiting requested review from twenzel

@cijothomas cijothomas Awaiting requested review from cijothomas

@g7ed6e g7ed6e Awaiting requested review from g7ed6e

@matt-hensley matt-hensley Awaiting requested review from matt-hensley

@rajkumar-rangaraj rajkumar-rangaraj Awaiting requested review from rajkumar-rangaraj

@utpilla utpilla Awaiting requested review from utpilla

@CodeBlanch CodeBlanch Awaiting requested review from CodeBlanch

@IliaBrahinets IliaBrahinets Awaiting requested review from IliaBrahinets

@pcwiese pcwiese Awaiting requested review from pcwiese

At least 1 approving review is required to merge this pull request.

Assignees

@matt-hensley matt-hensley

@twenzel twenzel

@g7ed6e g7ed6e

@cijothomas cijothomas

@xiang17 xiang17

@rajkumar-rangaraj rajkumar-rangaraj

@IliaBrahinets IliaBrahinets

@pcwiese pcwiese

@CodeBlanch CodeBlanch

@utpilla utpilla

Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore comp:instrumentation.grpccore Things related to OpenTelemetry.Instrumentation.GrpcCore comp:instrumentation.process Things related to OpenTelemetry.Instrumentation.Process comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime documentation Improvements or additions to documentation infra Infra work - CI/CD, code coverage, linters perf Performance related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.