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

Provide runtime/framework info in gRPC C# user agent string #25889

Merged
merged 3 commits into from Apr 20, 2021

Conversation

jtattermusch
Copy link
Contributor

@jtattermusch jtattermusch commented Apr 6, 2021

Make the user-agent string generated by the grpc-csharp client more useful by including info about the .NET Framework version in use, TFM, process architecture etc.

Example values of user-agent string:

  • MacOS:
    Mono:
    grpc-csharp/2.38.0-dev (Mono 5.20.1.19; CLR 4.0.30319.42000; net45; x64) grpc-c/16.0.0 (osx; chttp2)
    .NET Core
    grpc-csharp/2.38.0-dev (.NET Core 4.6.27617.05; CLR 4.0.30319.42000; netstandard2.0; x64) grpc-c/16.0.0 (osx; chttp2)

  • Linux:
    Mono:
    grpc-csharp/2.38.0-dev (Mono 6.12.0.122; CLR 4.0.30319.42000; net45; x64) grpc-c/16.0.0 (linux; chttp2)
    .NET Core
    grpc-csharp/2.38.0-dev (.NET Core 4.6.27019.06; CLR 4.0.30319.42000; netstandard2.0; x64) grpc-c/16.0.0 (linux; chttp2)

  • Windows:
    .NET Framework ("full framework")
    grpc-csharp/2.38.0-dev (.NET Framework 4.7.3163.0; CLR 4.0.30319.42000; net45; x64) grpc-c/16.0.0 (windows; chttp2)
    .NET Core:
    grpc-csharp/2.38.0-dev (.NET Core 4.6.27317.03; CLR 4.0.30319.42000; netstandard2.0; x64) grpc-c/16.0.0 (windows; chttp2)

  • Values on .NET Core 3.x and newer:
    grpc-csharp/2.38.0-dev (.NET Core 3.1.8; CLR 3.1.8; netstandard2.0; x64) grpc-c/16.0.0 (osx; chttp2)
    grpc-csharp/2.38.0-dev (.NET 5.0.0; CLR 5.0.0; netstandard2.0; x64) grpc-c/16.0.0 (osx; chttp2)

Known limitations:

  • Detecting .NET framework version is a tricky subject, especially for older framework versions. This PR tries to get the best signal possible without resorting to tricky and fragile logic of "fixing" wrong info provided by some of the framework versions.
  • Note that for .NET Core 2.x, the RuntimeInformation.FrameworkDescription reports wrong version (it should be 2.x), but that's a known bug that is fixed in .NET Core 3.x. This wrong value on 2.x can still be interpreted correctly since no other supported .NET Core version reports 4.x. Also, .NET Core 2.x will be end of life in summer 2021.
  • "FrameworkDescription" is only available starting from .NET Framework 4.7.1+ (released in 2017). For the legacy .NET Frameworks 4.5, 4.6 and 4.6.1, an approximation of framework version can be determined from the CLR version. That seems fair enough, considering those framework versions are ancient today (and hopefully no one is using them anymore).

@jtattermusch jtattermusch force-pushed the csharp_user_agent_changes branch 2 times, most recently from 3deecb3 to 168b1a8 Compare April 7, 2021 10:01
@jtattermusch jtattermusch marked this pull request as ready for review April 7, 2021 10:04
@jtattermusch jtattermusch added lang/C# release notes: yes Indicates if PR needs to be in release notes labels Apr 7, 2021
@jtattermusch
Copy link
Contributor Author

CC @srini100

@jtattermusch
Copy link
Contributor Author

FYI @jskeet

Copy link
Contributor

@apolcyn apolcyn left a comment

Choose a reason for hiding this comment

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

LGTM, comments are minor

detailComponents.Add($"CLR {clrVersion}");
}

if (tfm != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

optional nit: I believe tfm can never be null except for in unit tests, so this null check may be unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right but I added the null check for consistency.


static string TryGetArchitectureString(CommonPlatformDetection.CpuArchitecture arch)
{
switch (arch)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we get rid of this switch statement and just call ToString() on these enums? Then, we also don't need to check if this is null when constructing the user agent (I believe the architecture will just say "Unknown", which may be useful anyways)

}
}

var result = string.Join(" ", parts.GetRange(0, i));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move this string.Join(" ", parts.GetRange(0, i)); assignment to line 105, just before breaking out of the loop? Then, we can keep i as a loop-local

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That actually wouldn't work when all the parts are acceptable strings (e.g. the string is just ".NET Framework 4.5" since if (!Regex.IsMatch(part, @"^[-.,+@A-Za-z0-9]*$")) would never be true.

@jtattermusch
Copy link
Contributor Author

Updated TryGetArchitectureString().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/C# release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants