Improve properties capture for the sendErrorData method

[ilia-db]

Hey, I'm using this package and noticed that all unhandled errors reported by it don't have any common properties (like extension or vscode versions).

It seems like sender.sendErrorData doesn't always receive data objects with the properties field, in which case common properties that the vscode usually supplies to all events or errors will not be sent.

In the case of unhandled errors vscode itself calls sender.sendErrorData method providing the data object without properties field.

Internally (and publicly through TelemetryLogger extension API) vscode seem to always treat data argument as Record<string, any> type. It acknowledges that properties might exist while mixing in common props, but it doesn't create such data objects itself.

This PR accounts for that and fallbacks to using full data object if the properties field doesn't exist, fixing the reporting of unhandled errors.

[ilia-db]

@microsoft-github-policy-service agree company="Databricks"

[lramos15]

@Joouis this seems to be solving the same problem as microsoft/vscode#193208 but one in the module in one in core.

I was curious if either of you had preferences as to where the solution lived. I quite like this one as I feel like it's pretty clean. Downside being that sendErrorData now has a different call signature then the rest

[ilia-db]

I might be mistaken, but to me it looks like the linked PR in the VSCode core is not backwards compatible - it only mixes common properties to the data.properties object. It's ok for this telemetry package, but there might be others who would expect the common properties to be mixed into the the data object itself (which is the current behaviour when the properties field doesn't exist, and it's also mentioned in the VSCode API docs for additionalCommonProperties option).

Downside being that sendErrorData now has a different call signature then the rest

True, but sendErrorData isn't publicly exposed by this package, and public sendTelemetryErrorEvent method is the same.

[Joouis]

@lramos15 This can work as I thought of it at beginning, and I fully understand the compatibility concern. I don't prefer the idea because this is more like clean shit by downstream comsumers, letting all telemetry packages adapt to this different data structure design?

I would say sendErrorData(exception: Error, data?: SenderData | Record<string, any>) is a compromise, and could be difficult to understand for the future maintainers.

Just my 2 cents, still respect @ilia-db 's and your contribution.

[lramos15]

@Joouis I created some new test cases just to validate the VS Code side is working as expected and I believe it is.

The issue here is that the telemetry API doesn't know what special format your sender needs because it supports more than just app insights. Some might want all data at top level and others might want it split into properties and measurements. We try not to manipulate the data structure too much, which is why I believe the manipulation should be done by the library implementer as they know what the end ingestion result needs to be. Maybe the mistake in the API design was trying to manipulate the data structure at all as now we do have some app insights specific knowledge and not instead returning common properties separate for the sender to mixin.

Anyways my stance is that this is the better and more consistent approach that doesn't change the expected behavior of the API.