Jwilaby/#1185 app insights #1192

justinwilaby · 2018-12-20T21:59:51Z

resolves #1185

coveralls · 2018-12-20T22:09:41Z

Pull Request Test Coverage Report for Build 1632

52 of 56 (92.86%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.09%) to 58.359%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/app/main/src/commands/connectedServiceCommands.ts	0	2	0.0%
packages/app/main/src/services/appInsightsApiService.ts	42	44	95.45%

Files with Coverage Reduction	New Missed Lines	%
packages/app/client/src/data/sagas/servicesExplorerSagas.ts	1	70.11%

Totals
Change from base Build 1624:	0.09%
Covered Lines:	5048
Relevant Lines:	7781

💛 - Coveralls

cwhitten · 2018-12-20T22:22:00Z

packages/app/main/src/services/appInsightsApiService.spec.ts

+  return fetch;
+});
+
+const mockResponseTemplate = [


is there a notion of a fixtures directory in the emulator codebase? It would be nice to have this exportable and possibly held somewhere general for re-use, if it's reasonable to possibly need it again in a different test file.

We do not have fixtures in this codebase since there isn't a lot of opportunity to reuse mock data other than in the test it applies to. In this case, these are specific to the test suite and cannot be reused (just the variable name is reused).

FWIW I still think it's plenty valuable to remove this type of data from the test file for readability if nothing else. Plus it gives other developers a place to look to see if they're working on some shape that may be used already.

Ok, should I do this for all the others (~25+)so we can be consistent throughout the app?

cwhitten · 2018-12-20T22:23:22Z

packages/app/main/src/services/appInsightsApiService.ts

+    }
+
+    // 2. Retrieve the app insights components
+    yield { label: 'Retrieving Application Insights Components from Azure…', progress: 50 };


are these yielded only for the tests?

Take a look at the consumer of this generator. Those yields are to indicate progress.

I see. Is it common for a service like this to have presentational opinions on the state it emits? I could see a better pattern something like emitting status codes that the caller can respond to. This gives more control over the view to the caller.

So instead of emitting text, emit status codes that map to text in the consumer? Seems like an extra step that moves the responsibility of interpreting state onto the consumer. The idea here is to keep the consumer simple and dumb so it can manage any number of iterators. The service then is burdened with managing its own state as yielded output from the generator.

A scenario off to top of my head would be if the view needed to special case the label for any reason based off of some state that the view has (or knowledge of the user, their locale/language settings, etc).

Since these use cases do not yet exist and the technical implementation isn't clear. Let's keep this as-is and revisit it once we have a strategy for localization. I'm skeptical about adding infrastructure when the value of doing so depends on a yet-to-be defined requirement. Let's define the requirement, then refactor to fit.

cwhitten · 2018-12-20T22:24:39Z

packages/app/main/src/services/appInsightsApiService.ts

+
+export class AppInsightsApiService {
+
+  public static* getAppInsightsServices(armToken: string): IterableIterator<any> {


this function is doing quite a lot. can it be factored into smaller pieces? the way you've numbered the sections of code could be a good place to start!

Nah, I thought about it but the function signatures would have a lot of repetitive args and would make debugging more difficult since I would have to yield to another generator. I'm not sure there is a lot to gain from breaking it out into smaller functions.

Another note is that we are in the process of evaluating how this data is retrieved. It could change on the next iteration to include a hierarchical data structure to show the relationship between Subscription, Resource Group and Finally the Resource itself.

Hmm, it smells to me. I would think that would be more reason to be thoughtful about the separation of concerns, to not throw it all away if there is a change is relation or organization. Why can't this be broken into the most reasonable and smallest unit of work, so that down the road it can very easily be recombined or factored without much churn in the live code or the test code?

Tangentially but I forgot to ask, what happens if any of the network activity in the parts that need it fails?

Reusable work units are sufficiently broken out in the AzureManagementApiService class. Each of the composed services provide a concrete implementation that aggregates various data to build the models that are delivered once the generator completes. As we iterate on design and a new use case requires a different approach, refactoring is trivial and a low LOE.

cwhitten · 2018-12-20T22:27:59Z

...ent/src/ui/shell/explorer/servicesExplorer/connectedServicePicker/connectedServicePicker.tsx

@@ -371,7 +371,7 @@ export class ConnectedServicePicker extends Component<ConnectedServicesPickerPro
    return (
      <>
        <a href="https://aka.ms/bot-framework-emulator-create-appinsights" className={ styles.paddedLink }>
-          Create a new Azure storage account
+          Create a new Application Insights Component


conventionally this should evaluate a JS string, no?

I'm not sure what you mean here. JSX treats text node children as strings so yes, in that sense it does evaluate to a JS string then used as a TextNode in the dom once rendered. Is this what you mean?

By that I mean most of the text rendered in the DOM is explicitly evaluated like this in the code:

{ 'Create a new Application Insights Component' }

Bracket notation is only necessary when you need to preserve whitespace at the end of linebreaks in code. e.g.

<p>Create a new Application <- This space here will be lost when transpiling Insights Component</p>

To fix this we use:

<p> { 'Create a new Application Insights '+ <- space is preserved 'Component' } </p>

Understood, a lot of codebases would enforce brackets always for consistency. Once we move to internationalization this won't matter, as every string will be require it.

cwhitten · 2018-12-20T22:31:04Z

packages/app/main/src/services/appInsightsApiService.ts

+  const { id, name, properties } = component;
+  const { InstrumentationKey, ApplicationId, TenantId } = properties;
+  const service = new AppInsightsService();
+  service.id = id;


instead of mutating AppInsightsService's prototype after initialization here would it make more sense to pass this data into the constructor and giving it a type?

Either way works. I find this to be cleaner and easier to read versus:

new AppInsightsService( { id, name, serviceName: name, tenantId: TenantId, // ... }}

But this is generally preference IMO.

Made the changes here for consistency.

cwhitten · 2018-12-20T22:39:42Z

packages/app/main/src/services/cosmosDbApiService.ts

@@ -97,8 +97,9 @@ export class CosmosDbApiService {
 }

 function buildServiceModel
-(account: AzureResource, cosmosDb: AzureResource, collection: { id: string }): CosmosDbService {
+(account: AzureResource, cosmosDb: AzureResource, collection: { id: string, _rid: string }): CosmosDbService {
  const service = new CosmosDbService();


Can CosmosDbService also take constructor arguments instead of adding to members its prototype after initialization? see L101 - L109

Out of curiosity, is there an advantage to doing so?

Passing a defined shape in the constructor of a class is a technique that gives you a few things:

It documents a contract to the caller much better than expecting the caller to just know what values it needs to append to the prototype to have the instance work correctly.

It makes tooling work better (autocomplete/documented function signatures, for example).

It allows one to specify default values or validate inputs before the caller does something bad trying to use it down the road.

..there are more merits that I could expand on if you would like

It's a pretty effective mechanism.

In this case, a source config object is optional in the constructor and no default values are specified. I can see something like this being helpful:

class CosmosDbService { constructor({endpoint = '', key='', database='', collection=''}) { super({endpoint, key, database, collection}); }

But the author of the schema choose not to mandate default values.

cwhitten · 2018-12-21T21:29:16Z

@justinwilaby I'd like to put a pin in this diff until I can catch up with design

cwhitten · 2018-12-31T17:52:02Z

packages/app/main/src/services/appInsightsApiService.ts

+    yield { label: 'Retrieving Api Keys from Azure…', progress: 75 };
+    const apiKeysRequests = appInsightsComponents.map(info => {
+      const { component } = info;
+      return fetch(`${ baseUrl + component.id }/apiKeys?api-version=2015-05-01`, req);


apiKeys?api-version=2015-05-01 should probably be added to https://github.com/Microsoft/BotFramework-Emulator/blob/35e5eecd8a786949f7e3d21c7966e81982e6b88b/packages/app/main/src/services/azureManagementApiService.ts#L80 and referenced here.

The apiKeys endpoint is not an AccountIdentifier since it does not return account related information.

Would a new enum be appropriate next to this one holding this ${slug}?${version} pair?

cwhitten · 2018-12-31T17:55:56Z

packages/app/main/src/services/appInsightsApiService.ts

+    yield { label: 'Retrieving Application Insights Components from Azure…', progress: 50 };
+    const req = AzureManagementApiService.getRequestInit(armToken);
+    const requests = subs.map(sub => {
+      const url = `${ baseUrl + sub.id }/providers/${ Provider.ApplicationInsights }/components?api-version=2015-05-01`;


${ baseUrl + sub.id }/providers/${ Provider.ApplicationInsights }/${AccountIdentifier.ApplicationInsights} ?

Different API versions perform different actions on the same endpoint. It is possible that 2 different API versions can be used on the same endpoint yielding different results so I am keeping them within the concrete implementation instead of an enumeration.

"components?api-version=2015-05-01" is the value currently assigned to AccountIdentifier.ApplicationInsights. You'd prefer to duplicate and hardcode it here?

cwhitten · 2018-12-31T18:44:54Z

packages/app/main/src/services/cosmosDbApiService.ts

+                           cosmosDb: AzureResource,
+                           key: string,
+                           collection: { id: string, _rid: string }): CosmosDbService {
+  return new CosmosDbService({


With an ARM token from the Resource Manager, what's keeping us from utilizing the published SDK's?

https://www.npmjs.com/package/azure-arm-cosmosdb

The dependency list is why. It also uses ms-rest which has an insane number of dependencies itself. Pulling in very large libraries to accomplish trivial tasks will only serve to slow down an already slow build/startup and to increase the app's overall size. We need to be really careful what we choose to include as a dependency and live by the rule: The best dependency is no dependency.

You mean this ms-rest? The one already in our dependency tree?

"ms-rest-js": { "version": "1.0.455", "resolved": "https://registry.npmjs.org/ms-rest-js/-/ms-rest-js-1.0.455.tgz", "integrity": "sha512-RUDnFFNhk4ZdvOACg0yfaxmp5OzNwUcTIwgh/rVBeuNzgA7hOoVh5zFW06XmOtaBHXL2Bu/vWoQtzloEUlv9tw==", "requires": { "axios": "^0.18.0", "form-data": "^2.3.2", "tough-cookie": "^2.4.3", "tslib": "^1.9.2", "uuid": "^3.2.1", "xml2js": "^0.4.19" } }

was there any benchmarking done on this decision? can we not treeshake any of the deadcode?

I did a quick size test, here is what these two sdk's produce:

~/src/cosmosdbtest $ cat package.json { "name": "cosmosdbtest", "version": "1.0.0", "description": "", "main": "index.js", "scripts": { "test": "echo \"Error: no test specified\" && exit 1" }, "author": "", "license": "ISC", "dependencies": { "azure-arm-appinsights": "^2.1.0", "azure-arm-cosmosdb": "^2.3.0", "install": "^0.12.2", "npm": "^6.5.0" } }

~/src/cosmosdbtest $ ls -lah total 192 drwxr-xr-x 5 chriswhitten staff 160B Dec 31 14:12 . drwxr-xr-x 12 chriswhitten staff 384B Dec 31 14:11 .. drwxr-xr-x 74 chriswhitten staff 2.3K Dec 31 14:12 node_modules -rw-r--r-- 1 chriswhitten staff 88K Dec 31 14:12 package-lock.json -rw-r--r-- 1 chriswhitten staff 354B Dec 31 14:12 package.json

ms-rest is being removed when we can get away with it. We already made the decision to use an isomorphic fetch implementation. Haphazardly including dependencies means we could end up with 4 different libraries that make rest calls.

Also, ms-rest- does not appear to support proxy settings so using it will introduce a regression that will not exist with the one in this PR.

Regarding the dependencies, remember that they are fractal and NPM downloads all child, grandchild, great gandchild, etc. dependencies. This is a list of dependencies from node_modules when the only entry in the package.json is azure-arm-cosmosdb:

As you can see, this is probably not something we should include just to gain a few trivial rest calls.

A few things:

I understand how npm resolves dependency trees

62 of the 80 dependencies listed exist in the main package already (77.5%)

Adding azure-arm-cosmosdb and azure-arm-appinsights appear to add very little in total bytes to the dependency directory

As I see it, bundle size is important to consider, but less important than traditional webapps. Nonetheless, my approach to considering additional dependencies is definitely not "haphazard".

The "trivial" REST calls are not what you should focus on. You should focus on how you've now added a burden of maintaining live code and test code around communication with these services as they age and improve. There is a litany of things they could change requiring us to change our implementation(s) and test code down the road that could have been as easy as a version bump.

As new developers come into this codebase, you've locked them in to looking at your implementation to understand how to use your interface with these services instead of referencing the evergreen documentation published by the services themselves on the web.

There are scenarios where it would be appropriate to roll our own REST client but I'm not seeing it in this case beyond the missing support in using proxy which I would like to understand more, but generally speaking the next time we have a decision like this to make lets consider what I mentioned above.

cwhitten · 2018-12-31T18:45:19Z

packages/app/main/src/services/appInsightsApiService.ts

+    ApplicationId: applicationId,
+    TenantId: tenantId
+  } = properties;
+  return new AppInsightsService({


With an ARM token from the Resource Manager, what's keeping us from utilizing the published SDK's?

https://www.npmjs.com/package/azure-arm-appinsights

I have taken this into consideration as part of the initial technical study and have chosen not to include this as a dependency. See my comments on cosmosDB as to why.

justinwilaby · 2019-01-09T13:31:41Z

Stale

justinwilaby added 2 commits December 20, 2018 12:47

#1185 - Added application insights to supported services

90ccb0f

resolves #1185 - Add application insights support

b559a42

justinwilaby requested review from corinagum, tonyanziano and cwhitten December 20, 2018 22:00

cwhitten requested changes Dec 20, 2018

View reviewed changes

Passing source object for new service instances

4bbc4dd

cwhitten reviewed Dec 31, 2018

View reviewed changes

justinwilaby mentioned this pull request Jan 8, 2019

Add some additional keybindings, improve re-opening an already open bot #1203

Merged

justinwilaby closed this Jan 9, 2019

tonyanziano deleted the jwilaby/#1185-app-insights branch March 18, 2020 18:19


		export class AppInsightsApiService {

		public static* getAppInsightsServices(armToken: string): IterableIterator<any> {

Jwilaby/#1185 app insights #1192

Jwilaby/#1185 app insights #1192

Conversation

justinwilaby commented Dec 20, 2018

coveralls commented Dec 20, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1632

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhitten Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhitten Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhitten Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

justinwilaby Dec 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhitten commented Dec 21, 2018

Choose a reason for hiding this comment

justinwilaby Dec 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwhitten Dec 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinwilaby commented Jan 9, 2019

coveralls commented Dec 20, 2018 •

edited

Loading

cwhitten Dec 20, 2018 •

edited

Loading

cwhitten Dec 20, 2018 •

edited

Loading

cwhitten Dec 20, 2018 •

edited

Loading

justinwilaby Dec 20, 2018 •

edited

Loading

justinwilaby Dec 31, 2018 •

edited

Loading

cwhitten Dec 31, 2018 •

edited

Loading