-
Notifications
You must be signed in to change notification settings - Fork 150
Add support for .NET 7 #3482
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
Add support for .NET 7 #3482
Conversation
Datadog ReportBranch report: ❄️ New Flaky Tests (1)
|
bouwkast
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me, a couple of comments, but also the build is failing
eb12809 to
ce59b09
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
OmerRaviv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| .SetTargets("BuildDependencyLibs") | ||
| ); | ||
|
|
||
| // The live debugger dependency lib _sometimes_ has to be x86/x64, and _sometimes_ has to be AnyCPU apparently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm honestly not clear on this, in terms of why would it have to sometimes be AnyCPU (should be just x86/x64). Having said that, I'm fine with merging this as is, since @shurivich has been planning to work on this in the near term and refactor the way this dependency works, so if this works, great - we can just leave it as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something has changed in the way the .NET SDK uses the target platform supplied in build/test. One of the other changes I had to make is removing the `SetTagetFramework()" from the integration tests, and running those as "AnyCPU", though we still run against x64/x86, and the samples are built against x64/x86. The whole thing is a bit of a mess tbh, not just the debugger side 😬 I'm trying to do some cleanup in a separate PR but it's tough...
tonyredondo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Do we have plans to re-enable those flaky tests in .NET 7.0?
| #if NET7_0_OR_GREATER | ||
| [SkippableTheory(Skip = "Flaky in .NET 7")] | ||
| #else | ||
| [SkippableTheory(Skip = "Flaky in .NET 7")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Skip = "Flaky in .NET 7") should be removed here right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops yeah, thanks. And yes, we should prioritise re-enabling the tests, just want people to be able to do that in parallel as this PR is blocking various other work
ce59b09 to
5d4bee4
Compare
This comment has been minimized.
This comment has been minimized.
daniel-romano-DD
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are a titan
This comment has been minimized.
This comment has been minimized.
GreenMatan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Great job! 💯
tracer/test/Datadog.Trace.ClrProfiler.IntegrationTests/Debugger/ProbesTests.cs
Show resolved
Hide resolved
...oInstrumentation/Logging/ILogger/DirectSubmission/LoggerFactoryConstructorNet7Integration.cs
Show resolved
Hide resolved
They moved assembly in .NET
It uses bash, which isn't available on the new runners (We could add it) but it isn't necessary to be a separate stage anyway
There's some weird stuff going on here - adding x64 in the build integration tests seems to break some links Also workaround some debugger weirdness (that I really don't understand), again related to paths with x64/x86 Same for the profiler integration tests, specifying the target messes things up
- The version conflict tests require updating the NuGet we use to support .NET - The debugger tests require revisiting, as many fail. Skipping to unblock - The IAST deduplication tests are flaky in .NET 7 - frequently don't record any spans. Skipping to unblock - DefaultLargeTransport tests are behaving very weirdly currently, I think this is an artifact of the new images, but not 100% sure
These are flaky right now as they're timing out, but it's not clear why. We'll refactor in the longer term, but for now hoping this will help
…l, due to a bug in coverlet/the testing sdk. We should reenable at a later point
84d61be to
7cb7429
Compare
Benchmarks Report 🐌Benchmarks for #3482 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored. Benchmark detailsBenchmarks.Trace.AgentWriterBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AppSecBodyBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.AspNetCoreBenchmark - Unknown 🤷 Same allocations ✔️Raw results
Benchmarks.Trace.DbCommandBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ElasticsearchBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.GraphQLBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.HttpClientBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.ILoggerBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.Log4netBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.NLogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.RedisBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SerilogBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.SpanBenchmark - Same speed ✔️ Same allocations ✔️Raw results
Benchmarks.Trace.TraceAnnotationsBenchmark - Same speed ✔️ Same allocations ✔️Raw results
|
Summary of changes
Reason for change
We want to support the latest version of .NET. Previously some integrations (notably for types in the BCL) were not instrumented.
Implementation details
HttpClientHandler/SocketsHttpHandlerMicrosoft.Extensions.Logging.ILoggerWebRequestnet7.0to testing target frameworksnet7.0to smoke testsMicrosoft.VisualStudio.Component.Windows10SDK.19041component installed locally to build the profiler after this changeTest coverage
There were some failing/flaky tests that I've disabled in this PR which we should investigate and re-enable ASAP
Profiler call failed with result Unspecified-Failure (80131351): pInfo->GetModuleInfo(moduleId, nullptr, 0, nullptr, nullptr, &assemblyId)VersionConflict2xTests.cs: The version conflict tests require updating the NuGet we use to support .NET 7DeduplicationTests.cs: The IAST deduplication tests are flaky in .NET 7 - frequently don't record any spans.LineProbeEmit100SnapshotsTestMethodThrowExceptionTestHasLocalsAndArgumentsInGenericNestedTypeOther details
This update highlighted an issue with tiered jit, which we will also need to address: #3479