Initial changes to add telemetry

[bhsubra]

Changes include:

• Basic infrastructure to add telemetry for feature usage
• Add telemetry for top level declaration snippet insertion
• Add telemetry for resource and module body snippet insertion

[StephenWeatherford]

nit: Rather long telemetry event name. Consider shortening?

Is it possible that later snippet insertions would not be top-level or declaration? I.e. SnippetInsertion not good enough?

[bhsubra]

We want to add telemetry for resource and module body snippet insertions as well. We could also do something like - SnippetInsertion/TopLevel
SnippetInsertion/ResourceBody
SnippetInsertion/ModuleBody

I think it would show up as vscode-bicep/SnippetInsertion/TopLevel

@MarcusFelling , do you have any suggestions?

[StephenWeatherford]

Not a big deal, you just have to deal with whatever names you end up with in queries.

[anthony-c-martin]

Would it be worth creating a static method to set the properties, rather than exposing the constructor - that way as we add telemetry events, they're all contained in one place, and enforcing more control over the structure of the dictionary?

Thinking something like:

var telemetryEvent = TelemetryEvent.CreateTopLevelDeclarationSnippetInsertion(name: resourceSnippet.Prefix)

[bhsubra]

@anthony-c-martin, I refactored this class a bit. Let me know what you think:
5078ab9

[anthony-c-martin]

I think you should be able to extend ExecuteCommandHandlerBase<TelemetryEvent> instead of ExecuteCommandHandler and avoid having to write your own deserialization logic in the Handle method.

[bhsubra]

Updated here:
ffbef4c

[bhsubra]

With omnisharp 19, looks like the base handler doesn't take in a ISerializer. So added back the deserialization logic. Let me know if I am missing something.

[bhsubra]

Updated the base class here:
62d6763

[anthony-c-martin]

Added a few comments, please take a look!

[bhsubra]

This is expected. There was a bug in SnippetsProvider. See corresponding change in SnippetsProvider.cs.

[anthony-c-martin]

Looks like the base class has been renamed:
https://github.com/OmniSharp/csharp-language-server-protocol/blob/1b6788df2600083c28811913a221ccac7b1d72c9/src/Protocol/Features/Workspace/ExecuteCommandFeature.cs#L118-L141

I still think it's worth using this rather than doing it ourselves, as that way you ensure that the exact same serializer is being used.

[bhsubra]

Updated here:
62d6763

Thank you!

[anthony-c-martin]

This is a generated file - please could you revert the changes to it?

[bhsubra]

Reverted all, but one change in ln 1. Not sure why it's complaining. VS IDE does some auto formatting. I tried copying it from a clean machine, still has a diff in ln 1

[anthony-c-martin]

[nit] we have JToken assertions - you should be able to use:

extensionData["eventName"].Should().DeepEqual(TelemetryConstants.EventNames.TopLevelDeclarationSnippetInsertion);
extensionData["properties"].Should().DeepEqual(new JObject {
    ["name"] = "res-aks-cluster",
});

[bhsubra]

Don't need this anymore. Cleaned up here:
62d6763

[anthony-c-martin]

I think you can use the typed handler here to avoid the dictionary serialization that TelemetryEventParams does:

options.OnTelemetryEvent<BicepTelemetryEvent>(telemetry => telemetryReceived.SetResult(telemetry));

[bhsubra]

Updated here:
62d6763

[anthony-c-martin]

Why not just return the data directly to avoid repetition:

await IntegrationTestHelper.WithTimeoutAsync(telemetryReceived.Task);

[bhsubra]

Updated here:
62d6763

[anthony-c-martin]

I think you can remove all of these assertions about the command, as you're validating them all at the very end - ultimately that's what matters.

[bhsubra]

Updated here:
62d6763

[anthony-c-martin]

I didn't explain myself well with the suggestion of implementing Create - what I was thinking was implementing a set of Create methods - e.g. something like:

public record BicepTelemetryEvent : TelemetryEventParams
{
    public string? EventName { get; set; }

    public Dictionary<string, string>? Properties { get; set; }

    public static BicepTelemetryEvent CreateTopLevelDeclarationSnippetInsertion(string name)
        => new BicepTelemetryEvent {
            EventName = TelemetryConstants.EventNames.TopLevelDeclarationSnippetInsertion,
            Properties = new() {
                ["name"] = name,
            },
        };

    public static BicepTelemetryEvent CreateResourceBodySnippetInsertion(string name, string type)
        => new BicepTelemetryEvent {
            EventName = TelemetryConstants.EventNames.ResourceBodySnippetInsertion,
            Properties = new() {
                ["name"] = name,
                ["type"] = type,
            },
        };

    public static BicepTelemetryEvent CreateModuleBodySnippetInsertion(string name)
        => new BicepTelemetryEvent {
            EventName = TelemetryConstants.EventNames.ModuleBodySnippetInsertion,
            Properties = new() {
                ["name"] = name,
            },
        };
}

This has the benefit of grouping all the telemetry 'schemas' in one place, and simplifies the telemetry raising code.

[bhsubra]

This is definitely cleaner. Thanks! Updated here:
62d6763

[bhsubra]

Reverted all but this change. Not sure why it's complaining here.

[anthony-c-martin]

Looks great!