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

CoreCLR version in profiler API #12125

Closed
iskiselev opened this issue Feb 26, 2019 · 24 comments
Closed

CoreCLR version in profiler API #12125

iskiselev opened this issue Feb 26, 2019 · 24 comments
Assignees
Labels
area-Diagnostics-coreclr enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@iskiselev
Copy link

Profiler API already exposed method that expect to be used to resolve version, ICorProfilerInfo3::GetRuntimeInformation.
Unfortunately there is no documentation that will map results from it to marketing CoreCLR releases.
Per my tests it returns on Windows:
4.0.22220 for CoreCLR 2.0.9
4.0.30319 for CoreCLR 2.1.8, 2.2.0

Should it return value unique for each CoreCLR release? Should there be another API to resolve coreclr version (if we are talking about profiler, it will be some version associated with coreclr.dll)? Is there any existing way to resolve this version cross-platform? coreclr.dll on windows has File Version and Product version, are there something similar on other OS?

Will #12124 fix version resolution in profiler API?

cc: @noahfalk

@hoyosjs
Copy link
Member

hoyosjs commented Feb 26, 2019

That API you mentioned looks for some defined variables to populate the info and generate the strings. However, the headers that contain them are pre-compiled. You can find the change that caused the changing strings here: dotnet/coreclr@2023be1#diff-56e523e96dfe1ef322266c2732ca3e72.

So I'd expect the API to continue doing exactly the same as it was doing before the Environment.Version change, as those haven't really changed. Also, the Environment.Version change is mostly a pretty good approximation of the version. At the time of building CoreCLR, we have no information of the version that we use for external customers, so we'd need the host to provide it somehow. However, for more stable releases this API should give you the numbers you need for almost every single practical use. You can see the details of what was done, what causes the dependency issues that make it hard, and the reasoning of what version gets returned here: dotnet/coreclr#22664 (comment)

As for an API for this, @noahfalk do you think we should expose surface area that provides the same functionality? I'm not sure we obtain that information from anywhere in Corelib now so we'd have to pipe it down to the runtime to replicate the new API

@jkotas
Copy link
Member

jkotas commented Feb 26, 2019

At the time of building CoreCLR, we have no information of the version that we use for external customers

The algorithm we use everywhere else (e.g. in Environment.Version) is:

  1. Get the product version set by the host. In CoreCLR unmanaged code, this would be LPCWSTR fxProductVersion = Configuration::GetKnobStringValue(W("FX_PRODUCT_VERSION"))
  2. If there is no version set by the host, fallback to use the version baked into binaries. We do have the marketing version (e.g. 3.0.0) available when building CoreCLR repo and we bake it into CoreLib already. This version may be slightly lower than FX_PRODUCT_VERSION (e.g. the product version may be 3.0.2, but CoreCLR may still be at 3.0.1 because of we skipped rebuilding it for 3.0.2 servicing).

I think the existing profiler API should be changed to use this same algorithm.

@sywhang sywhang self-assigned this Feb 26, 2019
@sywhang
Copy link
Contributor

sywhang commented Feb 26, 2019

I will change the existing ICorProfilerInfo3::GetRuntimeInformation to return version info that matches Environment.Version. @noahfalk If you have any problems with that, please let me know!

@noahfalk
Copy link
Member

noahfalk commented Feb 27, 2019

Aligning it with the algorithm used for Environment.Version sounds good to me. Thanks all for jumping in 👍

@iskiselev - is your goal for the API to get a marketing version string that you can display to users and nothing else? Or were you hoping to make other inferences from it such as "In version X, the profiler API Y has behavior Z". If its the latter case we may have (or be able to create) better options.

Thanks!

@iskiselev
Copy link
Author

@noahfalk, we are looking into it from APM point of view: profiler can be loaded by lot's of different application for which we should gather some information/statistics to be represented to users. So now we are interested in API that will give some string that can be used to identify CLR version on which we are executed on. It may be not marketing version, but it will be good if it will be some build number associated with only one particular version (and all values for public releases should be documented). I would say it is not a big problem if different servicing version will have the same version if CoreCLR was not rebuilt, though it will be better if API provide really unique version string. It may be beneficial if different builds (Ubuntu/Redhat/Windows/...) will be detectable by version information too, but we have no such need right now (or we can try to find some workarounds if it will be required, such as implement our own OS detection and guess CoreCLR version from it or so on).
We know that it will be possible to read this information on managed side (RuntimeInformation, Environment.Version), but it will be much more convenient for us if this information will be available through Profiler API at Initialization time.

As we also use Profiler API for real instrumentation, I can see that we may need some profiler API feature/behavior detection, but we have not get any task that required it. Previously you was able to say (more or less) about feature/behavior by maximum ICorProfilerInfoX supported by framework. With dotnet/coreclr#22617 I've see, that there may be need with additional behavior detection mechanics, but it not affects me now.

@jkotas
Copy link
Member

jkotas commented Feb 28, 2019

Changing the version from 4 to 3 will cause OpenCover to leak: https://github.com/OpenCover/opencover/blob/d94d935bfe77a48395096e17ab430a142b3f628b/main/OpenCover.Profiler/CodeCoverage.cpp#L389 (related to dotnet/coreclr#22851).

If we go ahead with this change, we should submit PR to OpenCover to fix this, and also notify on #9305 to ask other profilers to apply the same fix if necessary.

@noahfalk
Copy link
Member

noahfalk commented Mar 1, 2019

cc @davmason

Nice spotting @jkotas! We do also have our announcments issue and breaking change doc that we should use to spread awareness.

Previously the breaking change doc was at Documentation/Profiling/Profiler Breaking Changes.md but we lost it when dotnet/coreclr#22617 was reverted and so we'll need to bring at least that part back.

EDIT: I later realized #9305 Jan mentioned is the announcments issue - sorry to be repetitive : )

@noahfalk
Copy link
Member

noahfalk commented Mar 1, 2019

Thanks for the info @iskiselev - it sounds like the new API behavior is the right choice for you (and hopefully many other profiler implementers will feel the same way). We'll certainly need to help smooth over some rough edges like what @jkotas found.

@ww898
Copy link
Contributor

ww898 commented Apr 20, 2019

@sywhang Could you please tell us when will you make changes in ICorProfilerInfo3::GetRuntimeInformation? We really expect them, because our code based on equivalent versions (major + minor) in ICorProfilerInfo3::GetRuntimeInformation and Environment.Version.

@tommcdon tommcdon assigned davmason and unassigned sywhang Apr 23, 2019
@tommcdon
Copy link
Member

@sywhang Could you please tell us when will you make changes in ICorProfilerInfo3::GetRuntimeInformation? We really expect them, because our code based on equivalent versions (major + minor) in ICorProfilerInfo3::GetRuntimeInformation and Environment.Version.

Load balancing to @davmason who can comment on @ee898's question

@davmason
Copy link
Member

@ww898 can you elaborate on what issues this will cause if not implemented? Currently it is marked as a backlog item which means it's not guaranteed to make it for 3.0. Knowing what trouble this causes will help define the priority of getting it implemented.

@ww898
Copy link
Contributor

ww898 commented Apr 30, 2019

@davmason @sywhang I develop a library which allows controlling profiling of the current process through the API based on P/Invoke. The library supports the case when both CLR v2 and CLR v4 (loaded into a single process) use the API simultaneously. Each of the CLRs communicates with a specific profiler COM instance with the help of a token. The token is generated using major and minor parts of the Environment.Version (in the managed code) and using major and minor parts of the ICorProfilerInfo3::GetRuntimeInformation (in the native code). The major and minor version parts were the same for Environment.Version and ICorProfilerInfo3::GetRuntimeInformation till this moment.

Unfortunately, now I should write the .NET Core v3 detector to process the unsymmetrical version change. And, what is even worse, this 'dirty hack' should work only till you update the ICorProfilerInfo3 :: GetRuntimeInformation version (which is unpredictable).

@davmason
Copy link
Member

davmason commented May 1, 2019

@ww898 Could you use a workaround to unblock you? There are coreclr only interfaces, i.e. ICorProfilerInfo9/ICorProfilerInfo10, that you can QI for to know if you're on coreclr or not.

In general the work to update the version returned will happen, it's just a question of when. Knowing how necessary this is will help me prioritize.

@ww898
Copy link
Contributor

ww898 commented May 7, 2019

@davmason Please correct me if I am wrong. To detect .NET Core v3.0 I should:

  1. receive COR_PRF_CORE_CLR from ICorProfilerInfo3::GetRuntimeInformation;
  2. Using QI calls, verify either ICorProfilerInfo9 or ICorProfilerInfo10 exists.

Is this correct?
Should I check that the version which I get from ICorProfilerInfo3::GetRuntimeInformation is 4.0.30319?
And what's more important: When should I turn this whole stuff off? E.g., when ICorProfilerInfo11 goes out? What's the condition?

P.S. May be it is easier to fix the version of ICorProfilerInfo3::GetRuntimeInformation right now?

@ww898
Copy link
Contributor

ww898 commented May 11, 2019

@davmason I have just implemented following checking to detect .NET Core v3.0:

  1. Check COR_PRF_CORE_CLR
  2. Check that ICorProfilerInfo9 exist and ICorProfilerInfo10 doesn't exist

Well, it's a fail, my tests are red, because .NET Core v2.1 / .NET Core v2.2 have same pattern. We really need something more unique to detect .NET Core v3.0.

@davmason
Copy link
Member

ICorProfilerInfo10 will exist in the released version of 3.0. It is in daily builds already and will be in preview 6 and later versions. You can use that to check for 3.0. If you have ICorProfilerInfo9 it means at least 2.2 and ICorProfilerInfo10 means 3.0 or later.

The problem with changing the version API is it is a breaking change and it is late in the development cycle for 3.0. It would fix your issues but cause issues for other vendors (for example the OpenCover issue mentioned by Jan earlier). My plan is to change it early in the .net 5.0 cycle so that profiler vendors have more time to react, and since 5 > 4 means that it will avoid certain classes of bugs.

@ww898
Copy link
Contributor

ww898 commented Oct 17, 2019

Hi @davmason, .NET Core 3.1 preview1 is out. And the issue came back. How to discern 3.1 and 3.0 in profiler agent? ICorProfileInfo10 has both now... Could you please fix it with FX_PRODUCT_VERSION or update ndpversion_generated.h?

@davmason
Copy link
Member

@ww898 can you help me understand why you need to know if you are targeting 3.0 or 3.1? From a profiler perspective it seems like if the API surface area is the same it shouldn't matter.

@ww898
Copy link
Contributor

ww898 commented Oct 19, 2019

@davmason System.Environment.Version contains real CLR version since the beginning of .NET world. GetRuntimeInformation() followed the rule ... till .NET Core v3. Where the rule was broken. Why? No answer.

We are using the fact that GetRuntimeInformation() and System.Environment.Version are the same in our API to bind the profiler and managed part. We implemented (thanks for your help) the workaround with ICoreProfilerInfo10 for .NET Core v3.0. Hower it doesn't work for v3.1 because System.Environment.Version was changed again.

I don't understand your plans for GetRuntimeInformation(). The version always will be 4.0.30319 even for .NET Core 5? What meaning the version which GetRuntimeInformation() returns in that case? Should I don't use this method for getting the real CLR version any more?

@davmason
Copy link
Member

@davmason System.Environment.Version contains real CLR version since the beginning of .NET world. GetRuntimeInformation() followed the rule ... till .NET Core v3. Where the rule was broken. Why? No answer.

GetRuntimeInformation() has been broken for as long as coreclr has existed. Here is the output for 2.1 and 3.0:

3.0:
    clrInstanceId:   6
    runtimeType:     coreclr
    majorVersion:    4
    minorVersion:    0
    buildNumber:     30319
    QFEVersion:      0
    szVersionString: v4.0.30319
    
2.1 latest:
    clrInstanceId:   5
    runtimeType:     coreclr
    majorVersion:    4
    minorVersion:    0
    buildNumber:     30319
    QFEVersion:      0
    szVersionString: v4.0.30319

We are using the fact that GetRuntimeInformation() and System.Environment.Version are the same in our API to bind the profiler and managed part. We implemented (thanks for your help) the workaround with ICoreProfilerInfo10 for .NET Core v3.0. Hower it doesn't work for v3.1 because System.Environment.Version was changed again.

I don't understand your plans for GetRuntimeInformation(). The version always will be 4.0.30319 even for .NET Core 5? What meaning the version which GetRuntimeInformation() returns in that case? Should I don't use this method for getting the real CLR version any more?

I did not realize it was broken until late in the development cycle for 3.0 when it was reported to us. There are various profilers that were ported over from desktop that check GetRuntimeVersion to see if the v4 profiler features are available by checking if (majorVersion >= 4). Changing the version to correctly report 3 late in the cycle would be a breaking change for those profilers so it was left as is.

I have a work item to fix GetRuntimeInformation in 5.0 so it will match System.Environment.Version, so starting with 5.0 it will correctly report what version you are running on.

@ww898
Copy link
Contributor

ww898 commented Oct 30, 2019

Thanks @k15tfu and @davmason ! The ICorProfilerInfo11 will help me to temporary solve the issue with .NET Core 3.1. See: dotnet/coreclr#27512.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@ww898
Copy link
Contributor

ww898 commented Mar 17, 2020

Hi @davmason, just tested v5.0.0-preview1. The issue is still present: System.Environment.Version returns 5.0.0, but ICorProfilerInfo3::GetRuntimeInformation still 4.0.30319.

@ww898
Copy link
Contributor

ww898 commented Mar 18, 2020

I found new ICorProfilerInfo12. It helps me to detect .NET v5.0.

@davmason
Copy link
Member

Fixed in #35327

@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Diagnostics-coreclr enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants