Skip to content
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

[BUG] stopTrackEvent requires property values to be strings #2209

Closed
50Wliu opened this issue Dec 7, 2023 · 7 comments
Closed

[BUG] stopTrackEvent requires property values to be strings #2209

50Wliu opened this issue Dec 7, 2023 · 7 comments
Assignees
Labels
bug investigation required Further investigation or discussions required released - NPM
Milestone

Comments

@50Wliu
Copy link
Member

50Wliu commented Dec 7, 2023

Description/Screenshot
The README says that properties accepts any type: https://github.com/microsoft/ApplicationInsights-JS/blob/HEAD/README.md#sending-telemetry-to-the-azure-portal
However, stopTrackEvent is typed to only accept a { [key: string]: string } which is in direct contradiction with the above.

Steps to Reproduce

  • OS/Browser: Edge
  • SDK Version [e.g. 22]:
  • How you initialized the SDK: npm

Expected behavior
I can include anything I want in properties.

Additional context
Version 3.0.5.

@MSNev MSNev added the bug label Dec 7, 2023
@MSNev MSNev added this to the 3.0.x milestone Dec 7, 2023
@siyuniu-ms
Copy link
Contributor

Hi @50Wliu Based on my local test, the following call was successful:

appInsights.startTrackEvent("test99"); 
appInsights.stopTrackEvent("test99", {"a":3.14, b:"TEST",c:{apple:"banana"}, d:80000})

I have verified in my Azure portal, and the telemetry appears as expected.
image

We will update the README accordingly to include this example. The current one may cause confusion as it contains "properties: {}".

Your feedback is greatly appreciated. If there are any edge cases or further suggestions, please let us know.

@50Wliu
Copy link
Member Author

50Wliu commented Feb 9, 2024

Hi @siyuniu-ms, thanks for looking into this. I'm referring to the TypeScript types which seem to be incorrect in this case.
See:

public stopTrackEvent(name: string, properties?: { [key: string]: string; }, measurements?: { [key: string]: number; }) {

@siyuniu-ms
Copy link
Contributor

Hi @siyuniu-ms, thanks for looking into this. I'm referring to the TypeScript types which seem to be incorrect in this case. See:

public stopTrackEvent(name: string, properties?: { [key: string]: string; }, measurements?: { [key: string]: number; }) {

You are corret. This behavior is intentional. As you may have observed in the Azure portal, telemetry data is organized into two distinct tables: one for measurements and another for properties. When our telemetry is transmitted to Azure, we ensure that the fields are segregated based on their data types — numeric values are categorized as measurements, while strings and nested objects are categorized as properties.

While the current functionality has been validated through local testing, we acknowledge the importance of providing clarity on this process and ensuring consistency between different track function. As such, we plan to incorporate additional documentation and ensure consistency of converting customer properties.

@siyuniu-ms siyuniu-ms added fixed - waiting release PR Committed and waiting deployment investigation required Further investigation or discussions required labels Feb 9, 2024
@siyuniu-ms
Copy link
Contributor

PS example in #2268 may help for better understanding

@50Wliu
Copy link
Member Author

50Wliu commented Feb 9, 2024

Thanks, but I'm still confused. I understand that numbers, nested objects, etc. can be transmitted, but my point is that the typings in this repo forbid that. In #2268 one of your examples is appInsights.stopTrackEvent("test",{property: {p1:3, p2:4}}).

If I try this, I get the following TypeScript error:
image

@siyuniu-ms
Copy link
Contributor

interesting, I could run it on my side:
image


I also have a test on edge Version 121 using 3.0.5 version loaded from script and it works.

Could you provide a more detailed screenshot and also try to print out Microsoft.ApplicationInsights.ms$mod in dev tool?

@50Wliu
Copy link
Member Author

50Wliu commented Feb 12, 2024

I'm referring exclusively to the compile-time TypeScript types, not runtime behavior (which from your comments, I understand works correctly if you decide to ignore the TypeScript errors).

The following code snippet is sufficient to reproduce the issue when using @microsoft/applicationinsightsweb 3.0.8 and TypeScript 5.2.2.

import { ApplicationInsights } from "@microsoft/applicationinsights-web";
const appInsights = new ApplicationInsights({
  config: {
    connectionString: "the connection string",
  },
});
appInsights.loadAppInsights();
appInsights.stopTrackEvent("test", { property: { p1: 3, p2: 4 } }); // ts2332: Type '{ p1: number; p2: number; }' is not assignable to type 'string'.
E:\devprod.wave.analysis.ado [main+1 ~0 -0 !] ➜ npx tsc
src/Contributions/LinkProvider/test.ts:8:38 - error TS2322: Type '{ p1: number; p2: number; }' is not assignable to type 'string'.

8 appInsights.stopTrackEvent("test", { property: { p1: 3, p2: 4 } });
                                       ~~~~~~~~
Found 1 error in src/Contributions/LinkProvider/test.ts:8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug investigation required Further investigation or discussions required released - NPM
Projects
None yet
Development

No branches or pull requests

3 participants