-
Notifications
You must be signed in to change notification settings - Fork 240
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
XHR/Fetch enhancement - add additional telemetry from window.performance #1194 #1195
Conversation
@@ -12,24 +12,29 @@ export class Traceparent { | |||
private static DEFAULT_TRACE_FLAG = "01"; | |||
private static DEFAULT_VERSION = "00"; | |||
public spanId: string; | |||
public traceFlag: string = Traceparent.DEFAULT_TRACE_FLAG; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to inside the constructor and referenced as self.xxx as by default TypeScript adds this automatically to it's constructor definition as this.xxxx and the "this." can't be minified.
import { EventHelper } from './ajaxUtils'; | ||
import { Traceparent } from './TraceParent'; | ||
import dynamicProto from "@microsoft/dynamicproto-js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Major refactor to add XHR/Fetch window.performance additional metrics.
- Hiding private properties (by using dynamicProto)
- refactoring out duplicate code
Nett effect of this change on the
Dependency plugin - Unminified (browser) = additional code of just over 37kb
- Minified (browser) = 318 bytes LESS
AISKU
- Unminified (browser) = additional code of just under 38Kb
- Minified (browser) = 1081 bytes LESS
response?: Object | ||
} | ||
|
||
let strProperties = "properties"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using string property lookups for multiple references to support and improve minification.
} | ||
|
||
/** @ignore */ | ||
function _populatePerfData(ajaxData:ajaxRecord, dependency:IDependencyTelemetry) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New perf data
|
||
export class XHRMonitoringState { | ||
public openDone: boolean = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As eariler, moving populating the values so that Typescript doesn't place this in the constructor as "this.xxxx" and instead we can use self.xxxx
public completed = false; | ||
public requestHeadersSize = null; | ||
public requestHeaders = null; | ||
public ttfb = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed as not used
this.spanID = spanID; | ||
this._logger = logger; | ||
public getAbsoluteUrl(): string { | ||
// @DynamicProtoStub -- DO NOT add any code as this will be removed during packaging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding so that TypeScript will generate a d.ts file the same as previously, however, the Rush plugin will remove the generated stub during packaging.
e.g. TS will create the function
ajaxRecord.prototype.getAbsoluteUrl = function() { ... }
But we will remove as this is just code bloat as the entire name can't be minified here, but by defining in the constructor and using dynamicProto() we remove this excessive "ajaxRecord.prototype." but we still end up with a class that defines the "getAbsoluteUrl" function on the ajaxRecord prototype
@@ -49,6 +49,7 @@ | |||
"typescript": "2.5.3" | |||
}, | |||
"dependencies": { | |||
"@microsoft/dynamicproto-js": "^0.5.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like all instances of this can be made devDependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As part of consuming the module (dist-esm folder) this would be a dependency and not a just a devDependency...
} | ||
|
||
/** @ignore */ | ||
function _throwInternalCritical(ajaxMonitorInstance:AjaxMonitor, msgId: _InternalMessageId, message: string, properties?: Object, isUserAct?: boolean): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these wrappers needed? Assuming they are for better minification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For minification as they are called more than once so it removes several duplicate code definitions.
a91715d
to
8b801e1
Compare
7b9b718
to
404eda5
Compare
* @param callbacks - The callbacks to configure and call whenever the function is called | ||
* @param checkPrototype - If the function doesn't exist on the target should it attempt to hook the prototype function | ||
*/ | ||
export function InstrumentFunc(target:any, funcName:string, callbacks: IInstrumentHooksCallbacks, checkPrototype:boolean = true): IInstrumentHook { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these functions make sense in core? Would it not make sense to include them in dynamicproto package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They belong here as DynamicProto doesn't do anything like what these functions are doing.
@@ -92,6 +92,7 @@ export const _InternalMessageId = { | |||
InvalidEvent: 70, | |||
FailedMonitorAjaxSetRequestHeader: 71, | |||
SendBrowserInfoOnUserInit: 72, | |||
PluginException: 73 | |||
PluginException: 73, | |||
InstrumentationError: 74, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appear to have removed the usage I did have in an earlier iteration -- will remove.
It was an additional error callback to report that instrumenting failed.
404eda5
to
cefd4e2
Compare
if (serverTiming) { | ||
let server = {}; | ||
_arrForEach(serverTiming, (value, idx) => { | ||
let name = value[strName] || "" + idx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add some additional checks / conversion to make sure the name can be validly set as a key to avoid exceptions.
/** @ignore */ | ||
function _throwInternalWarning(ajaxMonitorInstance:AjaxMonitor, msgId: _InternalMessageId, message: string, properties?: Object, isUserAct?: boolean): void { | ||
ajaxMonitorInstance[strDiagLog]()[strThrowInternal](LoggingSeverity.WARNING, msgId, message, properties, isUserAct); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain what does this syntax mean here instance[]()[]()
? where is ajaxMonitorInstance["dialog"]
come from? Thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
instance"diagLog""throwInternal"
is effectively the same as this.
instance.diagLog().throwInternal(...)
The primary difference is that the first situation allows for better minification (only) when the quoted strings are used more than once as the variables holding the quoted strings can be minified to 1 (or 2 characters) while the direct names cannot.
I should also added that for minification you MUST use variables to hold the strings as using quote strings actually adds more characters than it removes, and uglify will actually convert direct quoted strings back to the full names anyway.
public processTelemetry(item: ITelemetryItem, itemCtx?: IProcessTelemetryContext) { | ||
this.processNext(item, itemCtx); | ||
} | ||
let _fetchInitialized:boolean = false; // fetch monitoring initialized |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference to put these variables in constructor than leave outside? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside the constructor they become object (this) level properties and therefore all references will need to address them as this.xxxxx
This leads to 2 problems (especially for private / internal properties).
- The names can no longer be minified and must always be referenced as this.xxx (or self.xxxx)
- They are now exposed and can be altered externally, part of the point of private properties is they should be internal to the implementation / class.
By moving them into the constructor the "scope" (visibility) of these variables is now limited to the within the constructor and any and all functions defined within the constructor.
Shorter answer
- Better minification
- Hiding private / internal properties, only used to track internal state.
cefd4e2
to
4951cc1
Compare
4951cc1
to
98d8f7a
Compare
No description provided.