Modify telemetry to contain trigger time as property

[karrtikr]

No description provided.

[karthiknadig]

Why is this name being changed?

[karrtikr]

Based on discussion with Courtney offline, it was mentioned that due to a bug the classification may have went to the wrong property bag Property<->Measures, hence changing name to avoid confusion and start afresh.

[cwebster-99]

Spoke with Luciana and it seems like this should be language_server.trigger_duration to match the constant set in

vscode-python/src/client/telemetry/constants.ts

Line 67 in 75ed73e

LANGUAGE_SERVER_TRIGGER_DURATION = 'LANGUAGE_SERVER.TRIGGER_DURATION',

might not be the main problem but could be contributing. Looks like with your changes it is correct based on the name change.

[karrtikr]

Yes, my new telemetry name matches it. You mentioned:

there may be a bug in the product code that provides classification, it has happened before where the classification went to the wrong property bag Property<->Measures

Based on which I changed the telemetry name. I think we should avoid using . so confusion like this does not happen in the future.

[cwebster-99]

I think the classification was addressed more so by adding "isMeasurement": true in the last PR. All that to say it looks like the changes you made in addition to the name change would address some of the other potential issues. We can try this!

[cwebster-99]

This is also passed as a Measure. We could remove this later so long as it shows up correctly as a Measure since Properties as stored as strings.

[cwebster-99]

LGTM - discussed with Kartik and Luciana offline. Hopefully this one works 🤞

[cwebster-99]

Do we need to pass the {} in the props parameter here? @luabud

[karrtikr]

We have other telemetry such as UNITTEST_RUN_ALL_FAILED where that is not needed, and it seems to work.

vscode-python/src/client/testing/testController/pytest/pytestController.ts

Line 303 in 3b6c47b

sendTelemetryEvent(EventName.UNITTEST_RUN_ALL_FAILED, undefined);

[luabud]

would it be tricky to make isFirstSession a property of this event? 🙈 if it is nvm, this works well. It'd just simplify the metric creation

[karrtikr]

I mentioned it to Courtney but My reason for adding FIRST_SESSION as a separate telemetry, rather than including it in EDITOR_LOAD is that EDITOR_LOAD will be sent every session, as opposed to FIRST_SESSION, so I figured we might be saving some bandwidth.

But I can add it to EDITOR_LOAD if that's preferred, just let me know :)

[cwebster-99]

Luciana mentioned it might be easier to create/filter metrics if this was a property in the editor_load and trigger_time events so if it is the first session we can query firstSession = True or something like that in those events.

I guess how difficult would it be to add as a property? If it would be a significant lift we can still work with it as designed.

[karrtikr]

No problem, I've added it as part of EDITOR_LOAD

[cwebster-99]

I think we should also add it to the language_server_trigger_time event as well so we can compare that startup time on first session too.

[karrtikr]

That'd be a bit tough, to pass that info all the way down to where language_server_trigger_time is sent. The way I'm thinking is that we filter the first session out using editor load for which we check the value of language_server_trigger_time.

[cwebster-99]

TY