-
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
Enable support for reusing plugins in multiple instances of AppInsights #1132 #1133
Conversation
cdb970e
to
082905a
Compare
@@ -150,7 +150,12 @@ export class ApplicationInsightsDeprecatedTests extends TestClass { | |||
if(this.successSpy.called) { | |||
let currentCount: number = 0; | |||
this.successSpy.args.forEach(call => { | |||
currentCount += call[1]; | |||
call[0].forEach(message => { | |||
// Ignore the internal SendBrowserInfoOnUserInit message (Only occurs when running tests in a browser) |
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 comment says, these tests where failing when running in a browser but passed fine from the command line -- this fixes that issue.
@@ -60,7 +60,7 @@ export class SenderTests extends TestClass { | |||
baseData: {} | |||
}; | |||
try { | |||
this._sender.processTelemetry(telemetryItem); | |||
this._sender.processTelemetry(telemetryItem, 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.
The context is optional as existing plugin implementation may not pass it, so null is a valid value.
And it's really only required once we have a plugin that uses this value (i.e. a Singleton/shared plugin implementation)
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.
is including the , null
necessary?
@@ -150,12 +150,8 @@ export class Sender implements IChannelControlsAI { | |||
*/ | |||
private _timeoutHandle: any; | |||
|
|||
private _nextPlugin: ITelemetryPlugin; |
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.
These will now be hidden, so any plugin which was relying on these internal fields WILL be broken.
I've added an accessor for the _logger -> log(), but the only real usage of the _nextPlugin should have been to check and call processTelemetry() which now has a helper processNext()
082905a
to
4f3a249
Compare
4f3a249
to
2a9705c
Compare
|
||
// The underlying plugin is either not defined or does not have a processTelemetry implementation | ||
// so we still want the next plugin to be executed. | ||
_nextPlugin.processTelemetry(env, itemCtx); |
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 case actually fixes an existing issue where if you added a new IPlugin which didn't implement the processTelemetry() between (based on the priority) 2 other ITelemetryPlugin instances the trailing chain would not get called and it would actually caused an exception (in the ChannelsController), this exception will no longer occur because the execution is now wrapped with this proxy.
if (!_nextPlugin || !_nextPlugin.hasRun()) { | ||
// Either we have no next plugin or the current one did not attempt to call the next plugin | ||
// Which means the current one is the root of the failure so log/report this failure | ||
itemCtx.log().throwInternal( |
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.
Try and log details about the actual plugin that is causing the issue
if (_nextPlugin && !_nextPlugin.hasRun()) { | ||
// As part of the failure the current plugin did not attempt to call the next plugin in the cahin | ||
// So rather than leave the pipeline dead in the water we call the next plugin | ||
_nextPlugin.processTelemetry(env, itemCtx); |
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.
Calling the next plugin (if present) so that the pipeline isn't dead and therefore the throwInternal() can still succeed.
_self._getTelCtx(itemCtx).processNext(env); | ||
}; | ||
|
||
_self._getTelCtx = (overrideCtx:ITelemetryItemContext = 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.
This needs work :-(
For proper backward compatibility I need to record and process the instance level nextPlugin chain when no telemetryItemContext is available.
This state can also occur when a plugin gets executed (that wasn't wrapped by the TelemetryPluginProxy) and doesn't pass the existing telemetry context (i.e. existing plugins)
shared/AppInsightsCore/src/JavaScriptSDK/BaseTelemetryPlugin.ts
Outdated
Show resolved
Hide resolved
_self.processNext = (env: ITelemetryItem) => { | ||
let nextPlugin = _nextProxy; | ||
|
||
// Avoid double executing the same plugin |
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.
Due to backward compatibility, we can't add this double execution check :-(
shared/AppInsightsCore/src/JavaScriptSDK.Interfaces/ITelemetryItemContext.ts
Outdated
Show resolved
Hide resolved
2a9705c
to
8e14af8
Compare
2b1d723
to
16b04e3
Compare
16b04e3
to
7a24b79
Compare
this._logger = core.logger; | ||
initialize(config: IConfiguration & IConfig, core: IAppInsightsCore, extensions: IPlugin[], pluginChain?:ITelemetryPluginChain) { | ||
super.initialize(config, core, extensions, pluginChain); | ||
let ctx = this._getTelCtx(); |
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.
Even though it's private, is there another name we can use other than "context"? This word already has meaning through the SDK. Maybe "chain" or "origin"? Not a blocker, since it can be changed in a different version
@@ -60,7 +60,7 @@ export class SenderTests extends TestClass { | |||
baseData: {} | |||
}; | |||
try { | |||
this._sender.processTelemetry(telemetryItem); | |||
this._sender.processTelemetry(telemetryItem, 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.
is including the , null
necessary?
7a24b79
to
db7bf82
Compare
/** | ||
* Internal helper to allow setting of the internal initialized setting for inherited instances and unit testing | ||
*/ | ||
protected setInitialized: (isInitialized: 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.
Latest version changes order of these 2 methods to pass linting failure
let itemCtx:IProcessTelemetryContext = currentCtx; | ||
if (!itemCtx) { | ||
let rootCtx = _rootCtx || new ProcessTelemetryContext(null, {}, _self.core); | ||
// tslint:disable-next-line: prefer-conditional-expression |
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.
Ignoring this rule in this case as I think it's clearer this way.
Latest version only address linting failures