Adding Attach and Feature Statbeats #799

hectorhdzg · 2021-07-26T22:29:58Z

No description provided.

AutoCollection/Statsbeat.ts

lzchen · 2021-07-26T22:42:25Z

AutoCollection/Statsbeat.ts

        if (process.env.WEBSITE_SITE_NAME) { // Web apps
            this._resourceProvider = Constants.StatsbeatResourceProvider.appsvc;
+            this._resourceIdentifier = process.env.WEBSITE_SITE_NAME;


@heyams Current specs says WEBSITE_SITE_NAME/WEBSITE_HOME_STAMPNAME for rpid. Does this mean the literal string (with the slash concatenating the two), or choose WEBSITE_SITE_NAME if available, otherwise WEBSITE_HOME_STAMPNAME?

I expect WEBSITE_SITE_NAME/WEBSITE_HOME_STAMPNAME. if stampName is empty, it will be "{WEBSITE_SITE_NAME}/". yes.

lzchen · 2021-07-26T22:48:09Z

AutoCollection/Statsbeat.ts

        } else if (process.env.FUNCTIONS_WORKER_RUNTIME) { // Function apps
            this._resourceProvider = Constants.StatsbeatResourceProvider.function;
+            if (process.env.WEBSITE_HOSTNAME) {
+                this._resourceIdentifier = process.env.WEBSITE_HOSTNAME;
+            }
        } else if (this._config) {


What if the application is not in any of these rps? We probably shouldn't be sending this stats at all.

rp has a default case: "unknown". like standalone apps.. it's not an attach scenario.

Right, so we should not be sending any of the "attach" metrics in this default case.

@heyams can you confirm we don't send attach Statbeat for unknown

I would like to track unknown as well.. i.e. non-attach vs. attach. that's a good data trend to show.

What do we populate for properties then? Do we just leave empty? Does metric have same name and fields?

heyams · 2021-07-27T02:39:30Z

AutoCollection/Statsbeat.ts

+                "language": this._language,
+                "version": this._sdkVersion
+            }
+            this._statbeatMetrics.push({ name: Constants.StatsbeatCounter.ATTACH, value: 1, properties: attachProperties });


attach statsbeat is every 15 mins. feature is every 24 hours. you might want to have a separate method for attach and one for feature.

@heyams For applications that exit before 24 hours, do we send a statsbeat telemetry upon exit?

@heyams Why attach is being sent every 15 minutes?, are you expecting rpId to be updated that often?

this is the same interval as the azure metadata service.

vm id can get changed frequently, which impacts rpId for vm. other attach scenarios is not an issue.

@heyams For applications that exit before 24 hours, do we send a statsbeat telemetry upon exit?

it will be nice. currently java doesn't do that.

It's not only about "nice". If applications exit, there may be cases where we never get the telemetry. We MUST flush before exit.

@hectorhdzg probably not blocking for this pr though

@lzchen we flush all telemetry on exit, but we do not create telemetry on exit, we don't want to do that, that will make sense with high priority telemetry, maybe something that actually let users know that the app stopped running, but that doesn't apply with Statsbeats

Interesting. Something to bring up at tomorrow's meeting.

…t/ApplicationInsights-node.js into hectorh/statsbeatattach

AutoCollection/Statsbeat.ts

heyams

One minor constant renaming.. other than that, looks good to me.

lzchen · 2021-07-27T23:49:52Z

AutoCollection/Statsbeat.ts

+            let attachProperties = {
+                "os": this._os,
+                "rp": this._resourceProvider,
+                "rpid": this._resourceIdentifier,


Curious, what will undefined serialize as? Empty string? This is for the case when rp == unknown

this._resourceIdentifier defaults to "unknown" so it supposed to never be undefined, are you seeing something here?
I believe undefined properties are dropped when serializing JSON, at least they are when running this in the browser
JSON.stringify({"test":undefined})
"{}"

So on non-attach scenario, the payload would look like: {"rp": "unknown"} with no "rpid" correct?

Actually rpid is also included, something like this {"rp": "unknown","rpid": "unknown"}

lzchen · 2021-07-27T23:52:24Z

AutoCollection/Statsbeat.ts

+                "language": this._language,
+                "version": this._sdkVersion,
+                "attach": this._attach,
+                "feature": this._features,


The "aad handling" is listed as a feature. Will stats be released as the next beta version?

@hectorhdzg is aad handling the same as tracking aad enablement? i check your repo.. look like this is not added anywhere to the feature list. it only happens in tests.

We only have AAD feature in beta branch, I'm planning to update beta with latest changes in main for next release including Statsbeat implementation

@hectorhdzg
So statsbeats will be released as the next beta package?

Statbeats will be part of both releases, beta and regular one

lzchen · 2021-07-27T23:54:28Z

Statsbeats metrics related to instrumentations have not been added yet correct? @hectorhdzg

lzchen · 2021-07-27T23:59:19Z

AutoCollection/Statsbeat.ts

+                "language": this._language,
+                "version": this._sdkVersion,
+                "attach": this._attach,
+                "feature": this._features,


@heyams @hectorhdzg Do we have a comprehensive list of all the features we will be tracking for each language? It would also be good to coordinate bit representations for common features across our sdks.

each language will have their own specific features.. tracking AAD enablement will be a common feature.. other than that, there is nothing else i can think of.

What about local storage? Standard metrics? Pre-aggregated metrics? Live metrics? All of these are/will be features that will be supported by all sdks so we should agree on a list.

@lzchen having a shared list of features will make easier the data consumption here, happy to add them once we agree on them

Statsbeat is designed only for Breeze endpoints. Live metrics is not tracked. Java hasn't implemented pre-aggregated metrics and standard metrics yet.
I am planning to create a new set of statsbeat metrics for Local Storage. As of now, AAD is the only common feature. Let's get the basic Statsbeat done first and we can add the rest as Statsbeat evolves.. A list to agree upon at this moment is premature. Please document it somewhere for future consideration. (cc @ramthi)

hectorhdzg · 2021-07-28T16:26:41Z

@lzchen we send instrumentation property in all network Statsbeats, I don't believe there is an "Instrumentation" specific one.

Adding Attach and Feature Statbeats

5dda7f9

hectorhdzg requested review from xiao-lix, MSNev and heyams July 26, 2021 22:29

Fix test

93549b7

hectorhdzg requested a review from lzchen July 26, 2021 22:35

hectorhdzg commented Jul 26, 2021

View reviewed changes

AutoCollection/Statsbeat.ts Show resolved Hide resolved

lzchen reviewed Jul 26, 2021

View reviewed changes

heyams reviewed Jul 27, 2021

View reviewed changes

hectorhdzg and others added 4 commits July 27, 2021 09:11

Merge branch 'develop' into hectorh/statsbeatattach

bce51c6

Addressing comments

75cd67b

Adding env variable to disable Statsbeat

7baa0a3

Merge branch 'hectorh/statsbeatattach' of https://github.com/Microsof…

b37a6e1

…t/ApplicationInsights-node.js into hectorh/statsbeatattach

heyams reviewed Jul 27, 2021

View reviewed changes

AutoCollection/Statsbeat.ts Outdated Show resolved Hide resolved

heyams approved these changes Jul 27, 2021

View reviewed changes

Addressing comments

7fb7568

lzchen reviewed Jul 27, 2021

View reviewed changes

lzchen approved these changes Jul 28, 2021

View reviewed changes

hectorhdzg merged commit 35f9c47 into develop Jul 28, 2021

hectorhdzg deleted the hectorh/statsbeatattach branch September 16, 2021 22:40

snyk-bot mentioned this pull request Jan 29, 2022

[Snyk] Upgrade applicationinsights from 1.8.10 to 2.2.0 gopuff/appinsights-logger#18

Merged

Adding Attach and Feature Statbeats #799

Adding Attach and Feature Statbeats #799

Conversation

hectorhdzg commented Jul 26, 2021

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

lzchen Jul 27, 2021 • edited

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

heyams left a 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

lzchen commented Jul 27, 2021

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

hectorhdzg commented Jul 28, 2021

lzchen Jul 27, 2021 •

edited