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

Optional telemetry gathering #1851

Closed

Conversation

nickspoons
Copy link
Member

@nickspoons nickspoons commented Jul 9, 2020

This PR adds command-line flag -wt/--want-telemetry-info to request calling dotnet --info for telemetry purposes, and to otherwise not gather this information. Fetching this data slows OmniSharp-roslyn startup, and fails in certain circumstances (see #1844).

The essence of the PR is that the --want-telemetry-info flag is included in OmniSharpEnvironment, and when it is not set, the _dotNetCli service is not passed in to ProjectManager. This essentially undoes the effects of PR #1820, which is where the _dotNetCli service began being passed in to ProjectManager.

cc @filipw (commenter on #1844) and @JoeRobich (author of #1820) - does this look like the right fix? Would you rather we look at doing it a different way?

Note that this new flag is to opt-out of extra info being collected. We discussed opting-in in #1844 but that is more complicated as it involves changes on the VScode side, which I'm not familiar with. (This PR requires changes on the Vim side, (which I can make) - as well as any other editors which choose to opt out).

Edit: the -nt/--no-telemetry-info opt-out flags have now been changed to opt-in flags, -wt/--want-telemetry-info

Fixes #1844

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@nickspoons
Copy link
Member Author

nickspoons commented Jul 9, 2020

There are failed tests but I can't see what the problem is - are these related to my code changes?

Edit: never mind, I force pushed and the tests passed.

@nickspoons nickspoons force-pushed the optional-telemetry-gathering branch from 26cdd6b to 20283e5 Compare July 9, 2020 22:05
@filipw
Copy link
Member

filipw commented Jul 10, 2020

thanks a lot

Note that this new flag is to opt-out of extra info being collected. We discussed opting-in in #1844 but that is more complicated as it involves changes on the VScode side, which I'm not familiar with. (This PR requires changes on the Vim side, (which I can make) - as well as any other editors which choose to opt out).

IMHO if we are adding the flag it should make the telemetry metadata opt-in. New OmniSharp versions have to be manually added to VS Code extension anyway, so during that PR it would just be a matter of adding the extra startup flag, I wouldn't see that as very invasive.

Not to mention that, since this is improving perf quite considerably, it is in-line with the overall philosophy of Omnisharp of having a lean core and features on top of it that need to be enabled.

I am sure @david-driscoll and @JoeRobich would agree

@nickspoons
Copy link
Member Author

Thanks @filipw, in that case I'll change the flag to opt-in some time this weekend.

@nickspoons
Copy link
Member Author

I have replaced the -nt/--no-telemetry-info opt-out flags with -wt/--want-telemetry-info opt-in flags

@nickspoons
Copy link
Member Author

Any chance you could give this a review, @JoeRobich, since it looks like you're getting ready for a release?

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

I am not against this change, but hope that reducing the requests for dotnet --info to once per workspace works as a solution. see #1857

@@ -10,5 +10,6 @@ public interface IOmniSharpEnvironment
string SolutionFilePath { get; }
string SharedDirectory { get; }
string[] AdditionalArguments { get; }
bool WantTelemetryInfo { get; }
Copy link
Member

Choose a reason for hiding this comment

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

nit: I know names are hard and this isn't a blocker but maybe IncludeTelemetryInfo instead

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to make that change. What about the flag names? --include-telemetry-info and -it ?

@nickspoons
Copy link
Member Author

I've just built #1857 and works well for me. I tried it in this repo and did a few restarts, it generally is loading all projects on my machine in 21-23 seconds. Comparing that (my local build) with v1.35.2 (from the github release) it seems very similar - maybe a second slower but my testing is not at all robust enough to confirm that - and it may also be due to me having debug symbols in my local build.

I will therefore close this PR in favour of #1857 - thanks very much @JoeRobich (and @filipw for the helpful feedback).

@nickspoons nickspoons closed this Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can no longer run under WSL
3 participants