-
Notifications
You must be signed in to change notification settings - Fork 572
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
Get accurate runtime version #1936
Conversation
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.
Hard for me to grasp the whole code here and whether we've actively tested this on different environments but deferring to you here
@@ -204,29 +214,11 @@ private string BuildStripeClientUserAgentString() | |||
{ "bindings_version", StripeConfiguration.StripeNetVersion }, | |||
{ "lang", ".net" }, | |||
{ "publisher", "stripe" }, | |||
{ "lang_version", RuntimeInformation.GetLanguageVersion() }, | |||
{ "lang_version", RuntimeInformation.GetRuntimeVersion() }, |
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.
Is using the same property the right decision here? Won't it skew our results and make it harder to inspect?
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.
The current value is more or less useless (it tells us the proportion of Framework vs. Core users, but no useful version numbers). I think we can start sending meaningful values and update our internal metrics to only look at the lang_version
field when the bindings version is >= 35.0.
{ | ||
Type monoRuntimeType = typeof(object).Assembly.GetType("Mono.Runtime"); | ||
// the following code assumes that .NET 4.5 is the oldest supported version |
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 don't fully grasp why we do this instead of sending servicingVersion
or sanitizing it ourselves?
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.
The "servicing version" includes patch/build numbers that are not particularly relevant, so this method (adapted from here) sanitizes the servicing version to an actual release version (https://docs.microsoft.com/en-us/dotnet/framework/migration-guide/versions-and-dependencies#version-information).
* Drop support for .NET Standard 1.2 (#1933) * Add support for passing parameters for all Delete methods * Make options optional * Remove conditional compilation macros (#1935) * Move to latest API version and remove deprecated properties * Get accurate runtime version (#1936) * Add support for `NextInvoiceSequence` on `Customer` Co-authored-by: remi-stripe <[email protected]>
r? @brandur-stripe @remi-stripe @richardm-stripe
cc @stripe/api-libraries
This is an attempt to finally get accurate runtime versions in the extended user agent.
Some context: getting the runtime version with .NET in a consistent manner that works across all different runtimes (Framework/Core/Mono...) and versions is ridiculously hard. It has become a lot easier in recent times (as of .NET Core 3.0,
RuntimeInformation.FrameworkDescription
finally returns a sane value), but unfortunately we need to support older versions. The whole point of having accurate runtime versions is to let us know when we can safely drop support for older versions.Thankfully, the BenchmarkDotNet project has largely solved the problem by throwing a lot of code at it. I've been meaning to use their solution for a while now, and even asked them to consider releasing this specific part as a separate library (they said no).
Until recently we were unable to use their code because we were still supporting .NET Standard 1.2. We've dropped support for that version in this integration branch (cf. #1933) based on limited telemetry that I introduced in #1925, so we can now copy BenchmarkDotNet's solution.
The code is fairly complex because of the number of cases to handle, and is not especially pretty, but it works. I haven't written any tests for it because its very nature makes it hard to test, but I've manually verified that it reports sane values with .NET Core 2.0, .NET Core 3.1, Mono (on macOS) and .NET Framework 4.8 (on Windows, using Appveyor).
The code is largely copied from these files: