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.

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

Added App Insights instrumentation. #1251

Merged
merged 1 commit into from Jan 25, 2019

Conversation

Projects
None yet
4 participants
@tonyanziano
Copy link
Contributor

tonyanziano commented Jan 24, 2019

Addresses #1083

===

This PR adds Instrumentation to the Emulator that will track usage to an App Insights instance. The events being tracked are outlined in the internal document.

Show resolved Hide resolved packages/app/client/src/commands/emulatorCommands.ts Outdated
@@ -80,8 +81,10 @@ export function* getArmToken(
PersistAzureLoginChanged,
persistLogin
);
CommandServiceImpl.remoteCall(TrackEvent, 'signIn_success');

This comment has been minimized.

@justinwilaby

justinwilaby Jan 24, 2019

Contributor

These remote calls should probably be yielded so exceptions can be handled.

This comment has been minimized.

@a-b-r-o-w-n

a-b-r-o-w-n Jan 24, 2019

Contributor

What would the caller want to do in the case of an error? I would actually expect this to not throw an error because we wouldn't want the app to crash or become degraded just because we failed to track an event.

This comment has been minimized.

@tonyanziano

tonyanziano Jan 24, 2019

Author Contributor

I have put code into the TelemetryService that will swallow any exception thrown by the Application Insights sdk because like Andy, I don't think the app should crash when we fail to collect data.

I didn't want to synchronously execute TrackEvent for the same reason. I believe tracking events should be more of a "background process."

This comment has been minimized.

@tonyanziano

tonyanziano Jan 24, 2019

Author Contributor

However, the command service could still throw, so would adding a catch like this to each of the command calls to TrackEvent be sufficient?

commandService.remoteCall(TrackEvent, ...).catch(e => void 0);

This comment has been minimized.

@a-b-r-o-w-n

a-b-r-o-w-n Jan 25, 2019

Contributor

That seems reasonable to me. I think your reasoning is solid about not needing to wait for the telemetry as well.

Show resolved Hide resolved packages/app/client/src/data/sagas/azureAuthSaga.ts Outdated
Show resolved Hide resolved packages/app/client/src/ui/editor/emulator/emulator.tsx Outdated
Show resolved Hide resolved packages/app/client/src/ui/editor/emulator/emulator.tsx Outdated
Show resolved Hide resolved .../app/client/src/ui/editor/emulator/parts/inspector/inspectorContainer.ts
Show resolved Hide resolved packages/app/client/src/ui/shell/mdi/tabBar/tabBarContainer.ts
Show resolved Hide resolved packages/app/client/src/ui/shell/mdi/tabBar/tabBarContainer.ts
Show resolved Hide resolved packages/app/client/src/ui/shell/navBar/navBarContainer.ts
Show resolved Hide resolved packages/app/main/src/main.ts Outdated
Show resolved Hide resolved packages/app/client/src/commands/emulatorCommands.ts Outdated
@@ -80,8 +81,10 @@ export function* getArmToken(
PersistAzureLoginChanged,
persistLogin
);
CommandServiceImpl.remoteCall(TrackEvent, 'signIn_success');

This comment has been minimized.

@a-b-r-o-w-n

a-b-r-o-w-n Jan 24, 2019

Contributor

What would the caller want to do in the case of an error? I would actually expect this to not throw an error because we wouldn't want the app to crash or become degraded just because we failed to track an event.

private onChangeCollectUsageData = (): void => {
this.setUncommittedState({
collectUsageData: !this.state.uncommitted.collectUsageData,
});

This comment has been minimized.

@a-b-r-o-w-n

a-b-r-o-w-n Jan 24, 2019

Contributor

This has the potential of getting out of sync with state. Not sure how to approach given that setUncommittedState only accepts an object. In any case, probably not a huge problem unless someone spams the checkbox.

This comment has been minimized.

@tonyanziano

tonyanziano Jan 24, 2019

Author Contributor

Your first comment echoes what I was thinking. I thought it would be best if the app would avoid crashing if something failed while trying to track telemetry. However, I still believe the error should be swallowed inside of the TelemetryService so that it doesn't bubble up to the Command Service and become uncaught.

This comment has been minimized.

@tonyanziano

tonyanziano Jan 24, 2019

Author Contributor

Re: second comment

What would be a better pattern?
Pass the event into the handler and read the checked value of the element?

Currently all the checkboxes in AppSettingsEditor use this same pattern, but I'm open to changing them to one that is more reliable and wouldn't be at risk of falling out of sync.

This comment has been minimized.

@justinwilaby

justinwilaby Jan 24, 2019

Contributor

This can be:

const state = Object.create({}, {
  collectUsageData: {
    get: () => !this.state.uncommitted.collectUsageData,
    enumerable: true // important since rest spread is used.
  }
}):
this.setUncommittedState(state);

Which will read from the state at the moment the getter is invoked.

This comment has been minimized.

@a-b-r-o-w-n

a-b-r-o-w-n Jan 25, 2019

Contributor

That suggestion might be a little too complex. I think in general, the better approach would be to refactor the setUncommittedState method to allow an updater function instead that gets invoked with previous state. But as I said, I don't think it will be an issue in this case unless someone spams the checkbox.

This comment has been minimized.

@tonyanziano

tonyanziano Jan 25, 2019

Author Contributor

I've gone with a refactor including Justin's suggestion for now. However, I think we could investigate your approach down the road because we need to refactor AppSettingsEditor.tsx anyway and write a test for it, which ideally would be done at the same time.

Show resolved Hide resolved packages/app/client/src/ui/editor/emulator/emulator.spec.tsx
@@ -56,6 +56,7 @@ export interface LogEntryProps {
setInspectorObjects?: (...args: any[]) => void;
reconnectNgrok?: () => void;
showAppSettings?: () => void;
trackEvent?: (name: string, properties?: { [key: string]: any }) => void;

This comment has been minimized.

@a-b-r-o-w-n

a-b-r-o-w-n Jan 24, 2019

Contributor

Maybe we should just add a type for trackEvent?


import { getSettings } from '../settingsData/store';

const INSTRUMENTATION_KEY = '631faf57-1d84-40b4-9a71-fce28a3934a8';

This comment has been minimized.

@a-b-r-o-w-n

a-b-r-o-w-n Jan 24, 2019

Contributor

Is this secret?

This comment has been minimized.

@tonyanziano

tonyanziano Jan 24, 2019

Author Contributor

It is the key that allows anybody to post events to our App Insights instance, yes. I've spoken to the Azure CLI team about how they implemented instrumentation and they expose the key to the public in the same manner.

There is an email thread I can forward to you if you're interested, but basically the rationale behind it is this:

  • This does not increase exposure to DDoS attacks, because even if we abstracted the key, someone could write a script or use an auto clicker to perform some instrumented action in the Emulator and hammer our App Insights instance if they wanted to.
  • Internal data is not exposed in any way through this key. This only gives the ability to push data into the App Insights instance.
  • We can just scrub and throw away bad / garbage events

This comment has been minimized.

@a-b-r-o-w-n

a-b-r-o-w-n Jan 25, 2019

Contributor

That's fine with me. Just checking.

Is there a common place for config values such as this? If not, we may consider creating one. Even a config package. No need to update if not, but something to keep in mind. (I can think of a few other things that could live in that module as well)

This comment has been minimized.

@tonyanziano

tonyanziano Jan 25, 2019

Author Contributor

No official config file as far as I know. We do have a .env file in the root for using Travis, but that's about as close as it gets.

// turn off extra instrmentation
.setAutoCollectConsole(false)
.setAutoCollectDependencies(false)
.setAutoCollectExceptions(false)

This comment has been minimized.

@a-b-r-o-w-n

a-b-r-o-w-n Jan 24, 2019

Contributor

This one could be interesting. Why do we not want to collect exceptions?

This comment has been minimized.

@tonyanziano

tonyanziano Jan 24, 2019

Author Contributor

I just read the docs and it looks like this tracks all uncaught exceptions to App Insights. I don't really see a reason why we shouldn't collect these, but it is out of scope for the instrumentation spec. We can always flip the flag down the road.

This comment has been minimized.

@a-b-r-o-w-n

a-b-r-o-w-n Jan 25, 2019

Contributor

👍

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jan 24, 2019

Pull Request Test Coverage Report for Build 1931

  • 83 of 150 (55.33%) changed or added relevant lines in 35 files are covered.
  • 4 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+2.8%) to 49.784%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/app/client/src/commands/botCommands.ts 4 5 80.0%
packages/app/client/src/commands/uiCommands.ts 2 3 66.67%
packages/app/client/src/ui/editor/emulator/emulator.tsx 5 6 83.33%
packages/app/client/src/ui/editor/emulator/parts/inspector/inspectorContainer.ts 3 4 75.0%
packages/app/client/src/ui/editor/emulator/parts/log/logEntryContainer.ts 1 2 50.0%
packages/app/client/src/ui/helpers/activeBotHelper.ts 6 7 85.71%
packages/app/client/src/ui/shell/navBar/navBarContainer.ts 1 2 50.0%
packages/app/main/src/appUpdater.ts 1 2 50.0%
packages/app/main/src/commands/botCommands.ts 3 4 75.0%
packages/emulator/core/src/facility/conversation.ts 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
packages/app/main/src/main.ts 1 0.0%
packages/app/main/src/globals.ts 1 0.0%
packages/app/client/src/hyperlinkHandler.ts 2 0.0%
Totals Coverage Status
Change from base Build 1917: 2.8%
Covered Lines: 4508
Relevant Lines: 8458

💛 - Coveralls

@tonyanziano tonyanziano force-pushed the toanzian/#1083-telemetry branch from d49cc6e to f31be94 Jan 25, 2019

@justinwilaby
Copy link
Contributor

justinwilaby left a comment

:shipit:

@tonyanziano tonyanziano force-pushed the toanzian/#1083-telemetry branch 2 times, most recently from bc85ff0 to 02294b6 Jan 25, 2019

@tonyanziano tonyanziano force-pushed the toanzian/#1083-telemetry branch from 02294b6 to 43ba5dc Jan 25, 2019

@justinwilaby justinwilaby merged commit fe620b3 into master Jan 25, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 49.784%
Details

@tonyanziano tonyanziano deleted the toanzian/#1083-telemetry branch Jan 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.