Jwilaby/#1185 app insights #1192
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| import '../fetchProxy'; | ||
| import { AppInsightsApiService } from './appInsightsApiService'; | ||
|
|
||
| const mockArmToken = 'bm90aGluZw.eyJ1cG4iOiJnbGFzZ293QHNjb3RsYW5kLmNvbSJ9.7gjdshgfdsk98458205jfds9843fjds'; | ||
| let mockArgsPassedToFetch = []; | ||
| let mockResponses; | ||
| jest.mock('node-fetch', () => { | ||
| const fetch = (url, headers) => { | ||
| mockArgsPassedToFetch.push({ url, headers }); | ||
| return { | ||
| ok: true, | ||
| json: async () => mockResponses.shift(), | ||
| text: async () => mockResponses.shift() | ||
| }; | ||
| }; | ||
| (fetch as any).Headers = class { | ||
| }; | ||
| (fetch as any).Response = class { | ||
| }; | ||
| return fetch; | ||
| }); | ||
|
|
||
| const mockResponseTemplate = [ | ||
|
There was a problem hiding this comment. is there a notion of a There was a problem hiding this comment. 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). There was a problem hiding this comment. 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. There was a problem hiding this comment. Ok, should I do this for all the others (~25+)so we can be consistent throughout the app? |
||
| { | ||
| // Subscriptions | ||
| value: [ | ||
| { | ||
| 'id': '/subscriptions/1234', | ||
| 'subscriptionId': '1234', | ||
| 'tenantId': '1234', | ||
| 'displayName': 'Pretty nice Service', | ||
| 'state': 'Enabled', | ||
| 'subscriptionPolicies': { | ||
| 'locationPlacementId': 'Public_2014-09-01', | ||
| 'quotaId': 'MSDN_2014-09-01', | ||
| 'spendingLimit': 'On' | ||
| }, | ||
| 'authorizationSource': 'Legacy' | ||
| }, | ||
| { | ||
| 'id': '/subscriptions/1234', | ||
| 'subscriptionId': '1234', | ||
| 'tenantId': '43211', | ||
| 'displayName': 'A useful service', | ||
| 'state': 'Enabled', | ||
| 'subscriptionPolicies': { | ||
| 'locationPlacementId': 'Internal_2014-09-01', | ||
| 'quotaId': 'Internal_2014-09-01', | ||
| 'spendingLimit': 'Off' | ||
| }, | ||
| 'authorizationSource': 'RoleBased' | ||
| } | ||
| ] | ||
| }, | ||
| // Components | ||
| { | ||
| value: [ | ||
| { | ||
| 'id': '/subscriptions/08a9411c/resourceGroups/myResourceGroup/' + | ||
| 'providers/microsoft.insights/components/TestAppInsights', | ||
| 'name': 'TestAppInsights', | ||
| 'type': 'microsoft.insights/components', | ||
| 'location': 'westus2', | ||
| 'kind': 'Node.JS', | ||
| 'etag': '0000', | ||
| 'properties': { | ||
| 'ApplicationId': 'TestAppInsights', | ||
| 'AppId': '70f773ca', | ||
| 'Application_Type': 'Node.JS', | ||
| 'Flow_Type': 'Redfield', | ||
| 'Request_Source': 'IbizaAIExtension', | ||
| 'InstrumentationKey': '2e1f4ec2', | ||
| 'Name': 'TestAppInsights', | ||
| 'CreationDate': '2018-11-20T17:29:18.0789365+00:00', | ||
| 'PackageId': null, | ||
| 'TenantId': '08a9411c', | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| value: [ | ||
| { | ||
| 'id': '/subscriptions/08a9411c/resourceGroups/myResourceGroup2/' + | ||
| 'providers/microsoft.insights/components/TestAppInsights', | ||
| 'name': 'TestAppInsights', | ||
| 'type': 'microsoft.insights/components', | ||
| 'location': 'westus2', | ||
| 'kind': 'Node.JS', | ||
| 'etag': '0000', | ||
| 'properties': { | ||
| 'ApplicationId': 'TestAppInsights', | ||
| 'AppId': '70f773ca2', | ||
| 'Application_Type': 'Node.JS', | ||
| 'Flow_Type': 'Redfield', | ||
| 'Request_Source': 'IbizaAIExtension', | ||
| 'InstrumentationKey': '2e1f4ec22', | ||
| 'Name': 'TestAppInsights2', | ||
| 'CreationDate': '2018-11-20T17:29:18.0789365+00:00', | ||
| 'PackageId': null, | ||
| 'TenantId': '08a9411c2', | ||
| } | ||
| } | ||
| ] | ||
| }, | ||
| // api-keys | ||
| { | ||
| value: [ | ||
| { id: '/subscriptions/1234/resourcegroups/container/providers/microsoft.insights/components/com/apikeys/53434' } | ||
| ] | ||
| }, | ||
| { | ||
| value: [ | ||
| { id: '/subscriptions/123456/resourcegroups/container/providers/microsoft.insights/components/com/apikeys/4532' } | ||
| ] | ||
| } | ||
| ]; | ||
|
|
||
| describe('The AppInsightsApiService', () => { | ||
| beforeEach(() => { | ||
| mockResponses = JSON.parse(JSON.stringify(mockResponseTemplate)); | ||
| mockArgsPassedToFetch.length = 0; | ||
| }); | ||
|
|
||
| it('should deliver AppInsightsService objects when the happy path is followed', async () => { | ||
| const result = await getResult(); | ||
| expect(result.services.length).toBe(2); | ||
| expect(result.code).toBe(0); | ||
| }); | ||
|
|
||
| it('should return an empty payload with an error if no subscriptions are found', async () => { | ||
| mockResponses = [{ value: [] }]; | ||
| const result = await getResult(); | ||
| expect(result).toEqual({ services: [], code: 1 }); | ||
| }); | ||
|
|
||
| it('should return an empty payload with an error if no components are found', async () => { | ||
| mockResponses[1] = mockResponses[2] = { value: [] }; | ||
| const result = await getResult(); | ||
| expect(result).toEqual({ services: [], code: 1 }); | ||
| }); | ||
| }); | ||
|
|
||
| async function getResult() { | ||
| const it = AppInsightsApiService.getAppInsightsServices(mockArmToken); | ||
| let result = undefined; | ||
| while (true) { | ||
| const next = it.next(result); | ||
| if (next.done) { | ||
| result = next.value; | ||
| break; | ||
| } | ||
| try { | ||
| result = await next.value; | ||
| } catch (e) { | ||
| break; | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| // | ||
| // Copyright (c) Microsoft. All rights reserved. | ||
| // Licensed under the MIT license. | ||
| // | ||
| // Microsoft Bot Framework: http://botframework.com | ||
| // | ||
| // Bot Framework Emulator Github: | ||
| // https://github.com/Microsoft/BotFramwork-Emulator | ||
| // | ||
| // Copyright (c) Microsoft Corporation | ||
| // All rights reserved. | ||
| // | ||
| // MIT License: | ||
| // Permission is hereby granted, free of charge, to any person obtaining | ||
| // a copy of this software and associated documentation files (the | ||
| // "Software"), to deal in the Software without restriction, including | ||
| // without limitation the rights to use, copy, modify, merge, publish, | ||
| // distribute, sublicense, and/or sell copies of the Software, and to | ||
| // permit persons to whom the Software is furnished to do so, subject to | ||
| // the following conditions: | ||
| // | ||
| // The above copyright notice and this permission notice shall be | ||
| // included in all copies or substantial portions of the Software. | ||
| // | ||
| // THE SOFTWARE IS PROVIDED ""AS IS"", WITHOUT WARRANTY OF ANY KIND, | ||
| // EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF | ||
| // MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND | ||
| // NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE | ||
| // LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION | ||
| // OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION | ||
| // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
| // | ||
| import { ServiceCodes } from '@bfemulator/app-shared'; | ||
| import { AppInsightsService } from 'botframework-config/lib/models'; | ||
| import { AzureManagementApiService, AzureResource, baseUrl, Provider, Subscription } from './azureManagementApiService'; | ||
|
|
||
| export class AppInsightsApiService { | ||
|
|
||
| public static* getAppInsightsServices(armToken: string): IterableIterator<any> { | ||
|
There was a problem hiding this comment. 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! There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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? There was a problem hiding this comment. Reusable work units are sufficiently broken out in the |
||
| const payload = { services: [], code: ServiceCodes.OK }; | ||
|
|
||
| // 1. get a list of subscriptions for the user | ||
| yield { label: 'Retrieving subscriptions from Azure…', progress: 25 }; | ||
| const subs: Subscription[] = yield AzureManagementApiService.getSubscriptions(armToken); | ||
| if (!subs) { | ||
| payload.code = ServiceCodes.AccountNotFound; | ||
| return payload; | ||
| } | ||
|
|
||
| // 2. Retrieve the app insights components | ||
| yield { label: 'Retrieving Application Insights Components from Azure…', progress: 50 }; | ||
|
There was a problem hiding this comment. are these yielded only for the tests? There was a problem hiding this comment. Take a look at the consumer of this generator. Those yields are to indicate progress. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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). There was a problem hiding this comment. 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. |
||
| const req = AzureManagementApiService.getRequestInit(armToken); | ||
| const requests = subs.map(sub => { | ||
| const url = `${ baseUrl + sub.id }/providers/${ Provider.ApplicationInsights }/components?api-version=2015-05-01`; | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. 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. There was a problem hiding this comment.
|
||
| return fetch(url, req); | ||
| }); | ||
| const appInsightsResponses: Response[] = yield Promise.all(requests); | ||
| const appInsightsComponents: { component: AzureResource, subscription: Subscription }[] = []; | ||
| let i = appInsightsResponses.length; | ||
| while (i--) { | ||
| const response = appInsightsResponses[i]; | ||
| if (!response.ok) { | ||
| continue; | ||
| } | ||
| const { value: components = [] }: { value: AzureResource[] } = yield response.json(); | ||
| const subscription = subs[i]; | ||
| components.forEach(component => appInsightsComponents.push({ component, subscription })); | ||
| } | ||
|
|
||
| if (!appInsightsComponents.length) { | ||
| payload.code = ServiceCodes.AccountNotFound; | ||
| return payload; | ||
| } | ||
|
|
||
| // 3. Retrieve the api-keys for each component | ||
| 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); | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. The There was a problem hiding this comment. Would a new enum be appropriate next to this one holding this |
||
| }); | ||
| const apiKeysResponses: Response[] = yield Promise.all(apiKeysRequests); | ||
| i = apiKeysResponses.length; | ||
| while (i--) { | ||
| const apiKeyResponse = apiKeysResponses[i]; | ||
| const { value: apiKeyInfos = [] } = yield apiKeyResponse.json(); | ||
| const { component, subscription } = appInsightsComponents[i]; | ||
| if (!apiKeyInfos.length) { | ||
| continue; | ||
| } | ||
| // The id field contains the apiKey in a url | ||
| // that needs to be extracted | ||
| const apiKeys = apiKeyInfos.map((keyInfo: { id: string }) => { | ||
| const parts = keyInfo.id.split('/'); | ||
| return parts[parts.length - 1]; | ||
| }); | ||
| payload.services.push(createAppInsightsService(component, subscription, apiKeys)); | ||
| } | ||
| return payload; | ||
| } | ||
| } | ||
|
|
||
| function createAppInsightsService(component: AzureResource, sub: Subscription, keys: string[]): AppInsightsService { | ||
| const { id, name, properties } = component; | ||
| const { InstrumentationKey, ApplicationId, TenantId } = properties; | ||
| const service = new AppInsightsService(); | ||
| service.id = id; | ||
|
There was a problem hiding this comment. instead of mutating There was a problem hiding this comment. 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. There was a problem hiding this comment. Made the changes here for consistency. |
||
| service.name = service.serviceName = name; | ||
| service.instrumentationKey = InstrumentationKey; | ||
| service.tenantId = TenantId; | ||
| service.resourceGroup = id.split('/')[4]; | ||
| service.apiKeys = keys.reduce((map, key, index) => (map[`key${ index }`] = key, map), {}); | ||
| service.applicationId = ApplicationId; | ||
| return service; | ||
| } | ||
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.
conventionally this should evaluate a JS string, no?
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'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?
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.
By that I mean most of the text rendered in the DOM is explicitly evaluated like this in the code:
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.
Bracket notation is only necessary when you need to preserve whitespace at the end of linebreaks in code. e.g.
To fix this we use:
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.
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.