additional telemetry for webExtension #317

ashishchoudhary001 · 2022-10-18T05:56:33Z

No description provided.

src/web/client/common/authenticationProvider.ts

src/web/client/telemetry/webExtensionTelemetry.ts

This reverts commit 4288bc9.

…om/microsoft/powerplatform-vscode into users/nityagi/Additional_telemetry_preview

joem-msft · 2022-10-31T23:07:03Z

src/web/client/telemetry/webExtensionTelemetry.ts

    } else {
-        _telemetry?.sendTelemetryErrorEvent(eventName);
+        _telemetry?.sendTelemetryException(new Error(), telemetryData.properties);


you probably need to throw the exception so that you'll have a call stack.
And should you add the errorMessage to the Error()?

joem-msft · 2022-10-31T23:10:00Z

src/web/client/telemetry/webExtensionTelemetry.ts

-        const errorMessages: string[] = [];
-        errorMessages.push(errorMessage);
-        _telemetry?.sendTelemetryErrorEvent(telemetryData.eventName, telemetryData.properties, telemetryData.measurements, errorMessages);
+        const error: Error = new Error(errorMessage);


again, throw the error so you get a stack trace on it.

joem-msft · 2022-10-31T23:23:40Z

src/web/client/common/localStore.ts

            }
-            sendAPISuccessTelemetry(requestUrl, new Date().getTime() - requestSentAtTime);
+            sendAPISuccessTelemetry(requestUrl, PORTAL_LANGUAGES, httpMethod.GET, new Date().getTime() - requestSentAtTime);


the success line is called no matter if the response was ok or not.
causing double telemetry
this pattern seems the same with other places too

joem-msft · 2022-10-31T23:26:35Z

src/web/client/common/remoteSaveProvider.ts

@@ -32,32 +34,32 @@ export async function saveData(
        }
        requestBody = JSON.stringify(data);
    } else {
-        sendAPIFailureTelemetry(requestUrl, 0, BAD_REQUEST); // no API request is made in this case since we do not know in which column should we save the value
+        sendAPIFailureTelemetry(requestUrl, entityName, httpMethod.PATCH, 0, BAD_REQUEST); // no API request is made in this case since we do not know in which column should we save the value
        showErrorDialog(localize("microsoft-powerapps-portals.webExtension.save.file.error", "Unable to complete the request"), localize("microsoft-powerapps-portals.webExtension.save.file.error.desc", "One or more attribute names have been changed or removed. Contact your admin."));
    }

    if (requestBody) {


this condition shouldn't be needed. place the contents inside the if block above.
The 'data' will never be falsey in that code path.

joem-msft · 2022-10-31T23:44:28Z

src/web/client/telemetry/webExtensionTelemetry.ts

 }

-export function sendAPITelemetry(URL: string, isSuccessful?: boolean, duration?: number, errorMessage?: string, eventName?: string) {
+export function sendAPITelemetry(URL: string, entity: string, httpMethod: string, isSuccessful?: boolean, duration?: number, errorMessage?: string, eventName?: string) {


this function looks to be a core implementation function of the other functions.
I would even consider not using this function at all and remove all the error vs success conditionals to make the code easier to understand and with less conditionals.

From the patterns of usage, it looks like you want to track an event to 'start' the http request, and either a Success or Failure event after the call returns.. And not all implementations seem to be doing this correctly either.

I would suggest creating a single helper method which normalizes the API calls and telemetry semantics. By making a utility for the 'pattern' and making the behavior more general and consistent. This will make for simpler code at call sites and not much given up by way of context.

This will simplify any usage of the fetch calls so that they are easy to implement, and you won't have issues with missing infor or typos from copy/paste errors.
you'll probably need to return something like undefined | Response for the cases where an exception was thrown. Optionally, you could turn all invalid responses into a new Error, that gets thrown, and then in the catch do your logging, but then re-throw the error so the caller can do the actual handling. This would allow for telemetry to be recorded at the lowest level, but callers wouldn't need to do their own logging of it; just catch and provide whatever dialog or ignore the error.

Another thing that we've done on the Power Apps Runtime team is that we created our own FetchError class, which the helpers throw. When callers see this error, they can assume that the telemetry has been logged and this error indicates additional information about the response in case custom logic is needed.

e.g. something like export async function apiFetchWithTelemetry(entityName: string, requestUrl: string, requestInit?: RequestInit): Promise<Response>

you'll probably also find no need to have the three utility methods for telemetry specific to APIs since the implementation can simply be in this single pattern function.

joem-msft · 2022-11-01T00:43:04Z

src/web/client/common/remoteFetchProvider.ts

@@ -51,11 +51,11 @@ export async function fetchDataFromDataverseAndUpdateVFS(

        if (!response.ok) {
            showErrorDialog(localize("microsoft-powerapps-portals.webExtension.fetch.authorization.error", "Authorization Failed. Please run again to authorize it"), localize("microsoft-powerapps-portals.webExtension.fetch.authorization.desc", "Try again"));


it's not guaranteed to be an authorization error at this point in the code. it only means it wasn't a 200-299 response code.

joem-msft · 2022-11-01T00:43:41Z

src/web/client/common/remoteFetchProvider.ts

@@ -75,7 +75,7 @@ export async function fetchDataFromDataverseAndUpdateVFS(
        else {
            showErrorDialog(localize("microsoft-powerapps-portals.webExtension.parameter.error", "One or more commands are invalid or malformed"), localize("microsoft-powerapps-portals.webExtension.parameter.desc", "Check the parameters and try again"));
        }
-        sendAPIFailureTelemetry(requestUrl, new Date().getTime() - requestSentAtTime, errorMsg);
+        sendAPIFailureTelemetry(requestUrl, entity, Constants.httpMethod.GET, new Date().getTime() - requestSentAtTime, errorMsg);


I'd move telemetry recording to before showing UX.

joem-msft · 2022-11-01T01:03:03Z

src/web/client/common/remoteFetchProvider.ts

@@ -75,7 +75,7 @@ export async function fetchDataFromDataverseAndUpdateVFS(
        else {


This condition will only work for English locales. You should probably update your code to detect specific statusCodes first. Or handle specific error types.

You would also likely benefit from using node-fetch instead so that you have guaranteed Error-derived exceptions vs strings being thrown. See: ERROR-HANDLING.md

joem-msft · 2022-11-01T01:06:14Z

src/web/client/common/remoteSaveProvider.ts

@@ -32,32 +34,32 @@ export async function saveData(
        }
        requestBody = JSON.stringify(data);
    } else {
-        sendAPIFailureTelemetry(requestUrl, 0, BAD_REQUEST); // no API request is made in this case since we do not know in which column should we save the value
+        sendAPIFailureTelemetry(requestUrl, entityName, httpMethod.PATCH, 0, BAD_REQUEST); // no API request is made in this case since we do not know in which column should we save the value


I would consider NOT using the sendAPIFailureTelemetry event for this kind of error since there's no actual API being called yet.
Record this simply as another error or perhaps throw so the caller knows this is an invalid input?
this seems more like a caller having invalid parameters vs an API issue.

joem-msft · 2022-11-01T01:07:39Z

src/web/client/extension.ts

@@ -22,7 +22,6 @@ export function activate(context: vscode.ExtensionContext): void {
    context.subscriptions.push(_telemetry);

    setTelemetryReporter(_telemetry);
-    sendInfoTelemetry("Start");


add the Start event back. This is required for our telemetry pipeline to work. 'Start' means 'session started'.
You should add on the user's pac CLI user id, as is done in the desktop client's extension.

Ashish-portalFxDev added 3 commits October 16, 2022 13:37

added logs for userId and included entity, methodType in API telemetry

af58604

replaced error telemetry event with exception telemetry

205610c

added comment for verification

0b12de3

ashishchoudhary001 commented Oct 18, 2022

View reviewed changes

src/web/client/common/authenticationProvider.ts Outdated Show resolved Hide resolved

ashishchoudhary001 commented Oct 18, 2022

View reviewed changes

src/web/client/telemetry/webExtensionTelemetry.ts Outdated Show resolved Hide resolved

ashishchoudhary001 commented Oct 18, 2022

View reviewed changes

src/web/client/telemetry/webExtensionTelemetry.ts Show resolved Hide resolved

tyaginidhi approved these changes Oct 21, 2022

View reviewed changes

Test run for telemetry

4288bc9

tyaginidhi requested a review from tehcrashxor October 26, 2022 07:27

tyaginidhi added 5 commits October 26, 2022 15:33

Revert "Test run for telemetry"

2828d06

This reverts commit 4288bc9.

Merge branch 'main' into users/nityagi/Additional_telemetry_preview

df1951b

Make preview changes to branch

75f2ed2

Merge branch 'users/aschoud/additional_telemetry' of https://github.c…

8287a75

…om/microsoft/powerplatform-vscode into users/nityagi/Additional_telemetry_preview

remove preview changes from Ashish's branch

087695d

tyaginidhi self-requested a review October 26, 2022 11:38

Updated telemetry for preview verification

5a4e1c6

tyaginidhi marked this pull request as ready for review October 31, 2022 05:43

tyaginidhi approved these changes Oct 31, 2022

View reviewed changes

amitjoshi438 approved these changes Oct 31, 2022

View reviewed changes

tyaginidhi merged commit ace23ac into main Oct 31, 2022

tyaginidhi deleted the users/aschoud/additional_telemetry branch October 31, 2022 06:13

joem-msft reviewed Nov 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

additional telemetry for webExtension #317

additional telemetry for webExtension #317

ashishchoudhary001 commented Oct 18, 2022

joem-msft Oct 31, 2022

joem-msft Oct 31, 2022

joem-msft Oct 31, 2022

joem-msft Oct 31, 2022

joem-msft Oct 31, 2022

joem-msft Nov 1, 2022

joem-msft Nov 1, 2022

joem-msft Nov 1, 2022

joem-msft Nov 1, 2022

joem-msft Nov 1, 2022

joem-msft Nov 1, 2022

		@@ -51,11 +51,11 @@ export async function fetchDataFromDataverseAndUpdateVFS(

		if (!response.ok) {
		showErrorDialog(localize("microsoft-powerapps-portals.webExtension.fetch.authorization.error", "Authorization Failed. Please run again to authorize it"), localize("microsoft-powerapps-portals.webExtension.fetch.authorization.desc", "Try again"));

		@@ -75,7 +75,7 @@ export async function fetchDataFromDataverseAndUpdateVFS(
		else {

additional telemetry for webExtension #317

additional telemetry for webExtension #317

Conversation

ashishchoudhary001 commented Oct 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment