Log telemetry item when parse or compilation options change

[davidwengier]

Fixes AB#1171932

This logs some basic telemetry about how many parsed trees there are, and whether there was an existing compilation, when compilation options, parse options or assembly name is changed.

This is for @panopticoncentral to identify wasted work during solution load.

[davidwengier]

ping @jasonmalinowski this is ready for re-review

[ghost]

Hello @davidwengier!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

[ghost]

Auto-approval

[sharwell]

Still reviewing performance impact

[sharwell]

❔ Why are we gathering this in telemetry instead of just capturing the information as part of ETL traces submitted with performance reports? See #38194 for an example of a related case.

[davidwengier]

I guess I don't know. I don't know what a "performance report" is. Is that something @panopticoncentral can read? If there is already a system in place for collecting this information then that's great, and I guess its a matter of telling Paul where to get that data?

My understanding was that this telemetry event would be used as part of other telemetry events that come from CPS, VS, possibly Nuget, to get a handle on what possible wasted work might be happening on solution load.

[panopticoncentral]

The thought was to be able to construct models of solution opens across the telemetry, rather than only being able to do it for explicitly captured traces.

[sharwell]

What steps are in place to ensure this telemetry can be disabled if it is observed to produce large amounts of data and/or have an observable impact on client load times? Roslyn aims for these operations to be efficient (i.e. throwing away a Compilation is part of the normal fast path), so putting a telemetry invocation on the path seems to defeat its goal.

[sharwell]

One early option would disabling this telemetry if IsFullyLoadedAsync is true:

roslyn/src/Workspaces/Core/Portable/Workspace/Host/Status/IWorkspaceStatusService.cs

Line 46 in 28b4508

Task<bool> IsFullyLoadedAsync(CancellationToken cancellationToken);

This minimizes the impact of telemetry collection to a worst-case scenario of slower solution opening, while allowing us to expand the collection in the future if we both 1) demonstrate that it is not expensive and 2) demonstrate that analysis is in place such that the additional data is likely to directly result in a better future product.

[davidwengier]

Definitely this is only needed for solution open initially, and I agree, the aim was to add a little bit of telemetry now and add more only if it proves useful. That's why this is limited to changes that don't happen often during "normal" use. I'm fine with adding an explicit check for fully-loaded-ness.

[davidwengier]

That is annoyingly async. Is this pattern acceptable here? I would assume we don't want to block anything here.

[sharwell]

Yes that pattern is fine. Also can file a bug for me to just make IsFullyLoadedAsync a synchronous method so this is handled automatically.

[sharwell]

@davidwengier Two follow-up issues to file:

1. Consider making IsFullyLoadedAsync a synchronous method
2. Consider providing a default implementation of IWorkspaceTelemetryService so people don't have to remember that the service could be missing

[davidwengier]

Thanks all!