Adding StatsBeat implementation #792
Conversation
AutoCollection/StatsBeat.ts
Outdated
| private _totalSuccesfulRequestCount: number = 0; | ||
| private _totalFailedRequestCount: number = 0; | ||
| private _retryCount: number = 0; | ||
| private _exceptionCount: number = 0; |
There was a problem hiding this comment.
how does node.js handle multiple threading? these availables are needed to be thread-safe?
There was a problem hiding this comment.
No need to worry about multiple threads in Javascript, this values must be good and counts will be associated by specific Telemetry Clients
Declarations/Constants.ts
Outdated
| } | ||
|
|
||
| export enum StatsBeatInstrumentation { |
There was a problem hiding this comment.
i would like to see how you can decode the long back to string on the kusto side? i guess you will have another map for the string -> 2 to the power of x.
Please add a test for encoding and decoding so that i have a rough idea how it looks like in kusto.
There was a problem hiding this comment.
you might also consider when it's exceeding 64 bits, a new enum will be required? java has BitSet which can hold more than 64 bits.
There was a problem hiding this comment.
We have around 8 instrumentations currently so we must be good, I believe signed number used by bitwise operations in JS is only 32 bits
Library/AzureVirtualMachine.ts
Outdated
|
|
||
| class AzureVirtualMachine { | ||
|
|
||
| public isVM: boolean; |
There was a problem hiding this comment.
how do you keep this running every 15 mins when it's from VM?
There was a problem hiding this comment.
I assumed VM info is never updated during runtime, I will update so meta data request is made each time Statsbeat or Heartbeat need the info
|
how does the config look like for statsbeat?
|
|
@heyams for Statsbeat here there is no configuration file available, everything is set as attributes or constants in Statsbeat class |
| private _getResourceProvider(callback: () => void) { | ||
| // Check resource provider | ||
| let waiting: boolean = false; | ||
| if (process.env.WEBSITE_SITE_NAME) { // Web apps |
There was a problem hiding this comment.
I thought rpId only exist in "attach" Statsbeat, this PR is only adding network Statbeats will add attach and feature in different PR
| value: this._statbeatMetrics[i].value, | ||
| properties: properties | ||
| }; | ||
| let envelope = EnvelopeFactory.createEnvelope(statsbeat, Contracts.TelemetryType.Metric, null, null, this._statsbeatConfig); |
There was a problem hiding this comment.
it looks like config is used to store statsbeat connection string only. can you just pass the connection string to createEnvelop? unless you will parse connection string from the json config file later?
There was a problem hiding this comment.
We are using same mechanism as other envelopes created in the SDK, I'm trying to avoid specific code paths just for Statbeat here, added connection string as static so it could be updated if we add JSON config later
| if (enabled) { | ||
| if (clients.length === 0) { | ||
| channel.subscribe<any>("azure-coretracing", subscriber); | ||
| if (statsbeat) { | ||
| statsbeat.addInstrumentation(StatsbeatInstrumentation.AZURE_CORE_TRACING); |
There was a problem hiding this comment.
never mind about the other comment about instrumentation.. i think name is a good indicate what each number is for.
AutoCollection/Statsbeat.ts
Outdated
| this._isInitialized = false; | ||
| this._statbeatMetrics = []; | ||
| this._config = config; | ||
| this._statsbeatConfig = new Config(STATSBEAT_CONNECTIONSTRING); |
There was a problem hiding this comment.
it would be nice if you can get statsbeatConnectionString from the input parameter Config, then you can add statsbeat test to the e2e testing suite :)
There was a problem hiding this comment.
maybe add a todo for later if you like.
There was a problem hiding this comment.
I created a task for that, current there is no way to configure in attach scenarios but it would be great to have something available
This reverts commit 415c952.
This reverts commit 640ac77.
This reverts commit 838d926.
No description provided.