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 #22845

Open
iskiselev opened this issue Feb 26, 2019 · 16 comments

Comments

@iskiselev
Copy link

commented Feb 26, 2019

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 #22844 fix version resolution in profiler API?

cc: @noahfalk

@hoyosjs

This comment has been minimized.

Copy link
Member

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: 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: #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 jkotas added this to the 3.0 milestone Feb 26, 2019

@jkotas

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Author

commented Feb 27, 2019

@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 #22617 I've see, that there may be need with additional behavior detection mechanics, but it not affects me now.

@jkotas

This comment has been minimized.

Copy link
Member

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 #22851).

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

@noahfalk

This comment has been minimized.

Copy link
Member

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 #22617 was reverted and so we'll need to bring at least that part back.

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

@noahfalk

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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 tommcdon added this to Needs Triage in .NET Core Diagnostics via automation Apr 23, 2019

@tommcdon

This comment has been minimized.

Copy link
Member

commented Apr 23, 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.

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

@tommcdon tommcdon moved this from Needs Triage to Backlog in .NET Core Diagnostics Apr 23, 2019

@davmason

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

commented May 14, 2019

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.

@tommcdon tommcdon modified the milestones: 3.0, Future Jun 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.