Skip to content

Hook up Telemetry LSP event and add telemetry event when users opt-out/in to features #1381

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

Conversation

TylerLeonhardt
Copy link
Member

@TylerLeonhardt TylerLeonhardt commented Nov 7, 2020

needs PowerShell/vscode-powershell#3053 for the full flow.

The LSP has a message called telemetry event. The omnisharp lib gives us a mechanism for sending this so this PR hooks that up.
It's important to note that this doesn't make any requests besides sending a message to the client (aka vscode) to do the work of sending a telemetry event off the box. This way PSES doesn't have to depend on any AppInsights dll or something like that.

The event that this adds is an event called NonDefaultPsesFeatureConfiguration that has the body of settings that have been switched off/on from the default value. For example, if the user turns off the PackageManagement prompt (which defaults to on... aka true) then an event is fired with payload: { PromptToUpdatePackageManagement: false } where false is the new value it is being set to aka the non-default value.

This also modifies the E2E test to check to see if telemetry is sent when PSSA is turned off.

I also fixed a couple build warnings by adding Async to the end of all the test methods while I was touching it (sorry it makes the diff more annoying :( )

@TylerLeonhardt TylerLeonhardt changed the title add telemetry sending Hook up Telemetry LSP event and add telemetry event when users opt-out/in to features Nov 10, 2020
@TylerLeonhardt TylerLeonhardt marked this pull request as ready for review November 10, 2020 00:21
@TylerLeonhardt
Copy link
Member Author

@david-driscoll I'd love your thoughts on this because I'm not quite happy with the experience of typing telemetry events. Right now the event takes an IDictionary<string, JToken> but ideally I can provide my own type and that gets serialized as my telemetry event... but I didn't see how to do that.

Co-authored-by: Robert Holt <[email protected]>
@david-driscoll
Copy link

you can make your own model with a custom element and things should "just work".

        [Parallel]
        [Method(WindowNames.TelemetryEvent, Direction.ServerToClient)]
        public class PsesTelemetryEventParams : IRequest
        {
            ...<properties>...
        }

and then send it using server.SendNotification(new TelemetryEventParams(), ct);

On the receiving side you would have do a little more work. options.OnNotification<PsesTelemetryEventParams>(WindowNames.TelemetryEvent, (@params) => {...});

@TylerLeonhardt
Copy link
Member Author

@david-driscoll mentioned offline that there will be some work done to the telemetry event so that it accepts a custom type similar to launch/attach requests.

So I'll hold off on the refactoring until then.

@TylerLeonhardt TylerLeonhardt merged commit d7c8ddc into PowerShell:master Dec 7, 2020
@TylerLeonhardt TylerLeonhardt deleted the todo-add-pses-telemetry-sending branch December 7, 2020 20:29
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.

3 participants