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.

Sign up for GitHub

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

Jump to bottom

created initial debug plugin #1297

Merged
merged 2 commits into from
Jul 2, 2020
Merged

created initial debug plugin #1297

merged 2 commits into from
Jul 2, 2020

Conversation

fudgepop01
Copy link
Contributor

  • added parent bubble
  • added copy feature for detailed view
  • refactor logger element, allow copy/pasting

@fudgepop01 fudgepop01 requested a review from a team as a code owner June 16, 2020 18:23
hectorhdzg
hectorhdzg reviewed Jun 16, 2020
View reviewed changes
extensions/applicationinsights-internplugin-js/uploadFilesToAzureStorage.ps1 Outdated
$cacheControlValue = "public, max-age=1800, immutable";
$contentType = "text/javascript; charset=utf-8";

function Show-Menu
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe you need this file unless you are planning to expose this in CDN

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are planning on uploading this to the CDN, so that it's easier to include for different sites, even if they are using the snippet.

hectorhdzg
hectorhdzg reviewed Jun 16, 2020
View reviewed changes
extensions/applicationinsights-internplugin-js/sample/public/jsfile.js Outdated
@@ -0,0 +1,61 @@
const myFunc = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this file to sample.js or main or something like that

hectorhdzg
hectorhdzg reviewed Jun 16, 2020
View reviewed changes
extensions/applicationinsights-debugplugin-js/src/InternPlugin.ts Outdated
this.trackers = trackers;
}

initialize(config: IConfiguration, core: IAppInsightsCore, extensions: IPlugin[], pluginChain?: ITelemetryPluginChain) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method code is quite big, consider to split in multiple methods for better readability

MSNev
MSNev reviewed Jun 16, 2020
View reviewed changes
extensions/applicationinsights-debugplugin-js/sample/src/index.js Outdated
@@ -0,0 +1,24 @@
const polka = require("polka");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we actually need this at all as you should be able to just load the index.html directly.
The only difference would be around cors related headers handling (which I recently auto disabled when the protocol is file://)

@fudgepop01 fudgepop01 force-pushed the MSDAReba/debugplugin branch 2 times, most recently from a4eed21 to dce9f5b Compare June 24, 2020 19:58
hectorhdzg
hectorhdzg reviewed Jun 25, 2020
View reviewed changes
extensions/applicationinsights-debugplugin-js/README.md Outdated

This README is broken down into several parts:

- **Supported Browsers**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add links to the actual sections here so user can navigate easier?

MSNev
MSNev reviewed Jun 30, 2020
View reviewed changes
extensions/applicationinsights-debugplugin-js/src/DebugPlugin.ts
propertiesExt = extensions[i];
}
else if (extensions[i].identifier === 'AppInsightsChannelPlugin') {
channelPluginExt = extensions[i];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to change this as part of the next checkin as this is not always the case.
The extensions used by the internal Microsoft Teams don't all use this channel.
Like wise with the Properties plugin.

MSNev
MSNev reviewed Jun 30, 2020
View reviewed changes
extensions/applicationinsights-debugplugin-js/src/DebugPlugin.ts
Comment on lines +182 to +198
if (CoreUtils.arrIndexOf(propertiesProtoFns, tracker) !== -1) { target = propertiesExt['__proto__'] }
else if (CoreUtils.arrIndexOf(analyticsProtoFns, tracker) !== -1) { target = analyticsExt['__proto__'] }
else if (CoreUtils.arrIndexOf(ajaxProtoFns, tracker) !== -1) { target = ajaxDependencyExt['__proto__'] }
else if (CoreUtils.arrIndexOf(diagLogProtoFns, tracker) !== -1) { target = diagLog['__proto__'] }
// special case for sender
else if (tracker === '_sender') { target = channelPluginExt }
else if (CoreUtils.arrIndexOf(channelProtoFns, tracker) !== -1) { target = channelPluginExt['__proto__'] }
else { continue; }
InstrumentFunc(target, tracker, {
req: _self.preProcessItem(tracker) as any as () => InstrumentorHooksCallback,
rsp: _self.postProcessItem(tracker) as any as () => InstrumentorHooksCallback
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems overly complex, as I though I built some of these protections into the InstrumentFunc, but we can review later.

MSNev
MSNev reviewed Jun 30, 2020
View reviewed changes
extensions/applicationinsights-debugplugin-js/src/components/debugBins.ts Outdated
@@ -0,0 +1,116 @@
export class DebugBinParent {
public el: HTMLDivElement;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these need to be public? (if not then perhaps use DynamicProto() or just create your own closure in the constructor)

@fudgepop01 fudgepop01 force-pushed the MSDAReba/debugplugin branch 4 times, most recently from d590383 to fbf5803 Compare June 30, 2020 22:38
MSNev
MSNev reviewed Jul 1, 2020
View reviewed changes
extensions/applicationinsights-debugplugin-js/sample/public/sample.js
// Application Insights Configuration
var configObj = {
instrumentationKey: "MyInstrumentationKey",
appId: "OSKAR",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

MSNev
MSNev reviewed Jul 1, 2020
View reviewed changes
extensions/applicationinsights-debugplugin-js/src/components/helpers.ts
this.el.appendChild(this.ObjectToElements(target, key, level));
}

// git push -f origin MSDAReba/debugplugin
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be remove

MSNev
MSNev reviewed Jul 1, 2020
View reviewed changes
extensions/applicationinsights-debugplugin-js/src/interfaces/ITelemetryPlugin.ts
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

export interface ITelemetryConfig {
Copy link
Collaborator

@MSNev MSNev Jul 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably call this IDebugPlugConfig, we can do this is a followup PR

MSNev
MSNev reviewed Jul 1, 2020
View reviewed changes
extensions/applicationinsights-debugplugin-js/README.md
});
appInsights.loadAppInsights();
```

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next PR we should include details about the config, in a table like some of the other readme's

@fudgepop01 fudgepop01 merged commit 8a00b03 into master Jul 2, 2020
@MSNev MSNev added this to the 2.5.6 milestone Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markwolff markwolff markwolff left review comments

@xiao-lix xiao-lix xiao-lix left review comments

@hectorhdzg hectorhdzg hectorhdzg left review comments

@MSNev MSNev MSNev approved these changes

@ramthi ramthi Awaiting requested review from ramthi

@lzchen lzchen Awaiting requested review from lzchen

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
2.5.6
Development

Successfully merging this pull request may close these issues.
5 participants
@fudgepop01 @markwolff @xiao-lix @hectorhdzg @MSNev