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

Platform detection: Handle .NET 6 #1528

Merged
merged 9 commits into from
Nov 4, 2021
Merged

Platform detection: Handle .NET 6 #1528

merged 9 commits into from
Nov 4, 2021

Conversation

gregkalapos
Copy link
Contributor

@gregkalapos gregkalapos commented Oct 18, 2021 •
edited by russcam
Loading

Closes #1513

@apmmachine
Copy link
Contributor

apmmachine commented Oct 18, 2021 •
edited
Loading

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-11-03T19:17:14.306+0000

  • Duration: 52 min 54 sec

  • Commit: e2e2e83

Test stats 🧪

Test Results
Failed 0
Passed 19756
Skipped 127
Total 19883

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

@russcam
Copy link
Contributor

russcam commented Oct 19, 2021

Should there be tests for .NET 6 in this PR too?

@gregkalapos
Copy link
Contributor Author

gregkalapos commented Oct 19, 2021 •
edited
Loading

Should there be tests for .NET 6 in this PR too?

PlatformDetectionTests.cs has some test, but that does not test for the version - only asserts that .NET is contained in Service.Runtime.Name. I think in general we should have dynamic test that work with .NET {Version} and not for specific version, so we don't have to do this PR in every major .NET update. I'll extend PlatformDetectionTests.cs to cover this better.

Update: done.

@russcam this is ready for review.

@gregkalapos gregkalapos requested a review from russcam October 19, 2021 19:31
russcam
russcam reviewed Oct 20, 2021
View reviewed changes
Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

I've had another look, and made some further suggestions

test/Elastic.Apm.Tests/NetCoreAndNet5Fact.cs Outdated
@@ -12,7 +12,9 @@ public sealed class NetCoreAndNet5Fact : FactAttribute
{
public NetCoreAndNet5Fact()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should update the name to NetCoreAndNetFact

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

src/Elastic.Apm/Helpers/PlatformDetection.cs Outdated
RuntimeInformation.FrameworkDescription.StartsWith(DotNet5Prefix, StringComparison.OrdinalIgnoreCase);
internal static readonly bool IsDotNet5OrNewer =
RuntimeInformation.FrameworkDescription.StartsWith(DotNetPrefix, StringComparison.OrdinalIgnoreCase) &&
!RuntimeInformation.FrameworkDescription.StartsWith(DotNetFullFrameworkDescriptionPrefix, StringComparison.OrdinalIgnoreCase) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be sufficient to check

  1. the FrameworkDescription is at least 6 characters in length
  2. starts with .NET
  3. the sixth character is a digit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that'd also make it. I changed it.

src/Elastic.Apm/Helpers/PlatformDetection.cs Outdated
@@ -13,7 +13,7 @@ namespace Elastic.Apm.Helpers
{
internal static class PlatformDetection
{
internal const string DotNet5Prefix = ".NET 5";
internal const string DotNetPrefix = ".NET ";
Copy link
Contributor

Choose a reason for hiding this comment

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

is the space at the end intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we use this as a prefix and we always look for .NET [Version].

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like PlatformDetection could use the constants defined in Runtime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's a good point - done.

@gregkalapos
Copy link
Contributor Author

Regarding IsDotNet5OrNewer vs IsDotNet: Initially I also thought let's just call it IsDotNet, but that can be super super confusing - usages look like: PlatformDetection.IsDotNet, and I think this does not help readability - I feel some people won't get that.

On the other hand I agree IsDotNet aligns better with the other properties. I feel it's not worth spending more cycle on this, so I changed it to IsDotNet (also changed other occurrences of this) and added a small comment to the property - with that this should be fine.

russcam
russcam reviewed Oct 21, 2021
View reviewed changes
src/Elastic.Apm/Helpers/PlatformDetection.cs Outdated
internal static readonly bool IsDotNet =
RuntimeInformation.FrameworkDescription.StartsWith(DotNetPrefix, StringComparison.OrdinalIgnoreCase) &&
RuntimeInformation.FrameworkDescription.Length >= 6
&& int.TryParse(RuntimeInformation.FrameworkDescription.Substring(5).Split('.')[0], out _);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
&& int.TryParse(RuntimeInformation.FrameworkDescription.Substring(5).Split('.')[0], out _);
&& char.IsDigit(RuntimeInformation.FrameworkDescription[5]);

char.IsDigit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thinking behind handling this as multiple characters was .NET 10 and newer (yeah, I know... it's a long time until then).

It'd still work if everything is fine by just checking the 1. character and ignoring the 2. This runs only once, so I think overhead should not matter here.

Anyways, I changed it to use char.IsDigit.

src/Elastic.Apm/Helpers/PlatformDetection.cs Outdated
@@ -13,7 +13,7 @@ namespace Elastic.Apm.Helpers
{
internal static class PlatformDetection
{
internal const string DotNet5Prefix = ".NET 5";
internal const string DotNetPrefix = ".NET ";
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like PlatformDetection could use the constants defined in Runtime?

@gregkalapos gregkalapos requested a review from russcam November 3, 2021 19:52
@gregkalapos gregkalapos merged commit 620abfa into elastic:master Nov 4, 2021
@gregkalapos gregkalapos deleted the PlatformDetectionNet6 branch November 4, 2021 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@russcam russcam russcam approved these changes

Assignees
No one assigned
Labels
agent-dotnet v1.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

.NET6 testing/support
3 participants
@gregkalapos @apmmachine @russcam