-
Notifications
You must be signed in to change notification settings - Fork 132
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
Configure page for Application Insights #2560
Conversation
} | ||
|
||
shouldRenderMonitorClassic(): boolean { | ||
public shouldRenderMonitorClassic(): boolean { |
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.
nit: can we make this a get function if it's just here to be used in the template code? Feels a little cleaner without () in the html #ByDesign
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.
return this._renderComponentName === ComponentNames.monitorClassic; | ||
} | ||
|
||
shouldRenderMonitorApplicationInsights(): boolean { | ||
public shouldRenderMonitorApplicationInsights(): boolean { |
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.
same here #Resolved
this.functionMonitorInfo.appInsightsResourceDescriptor === null) { | ||
errorEvent = { | ||
errorId: errorIds.applicationInsightsInstrumentationKeyMismatch, | ||
message: 'Application insights key mismatch.', |
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.
can't really tell where this is being used, but want to make sure its a logged item and not something that belongs in resources #Resolved
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.
actually looks like it belongs in resources because you translate it in other places #Resolved
<command displayText="{{ 'refresh' | translate }}" | ||
(click)="refreshMonitorClassicData()" | ||
iconUrl="image/reset.svg"></command> | ||
</command-bar> | ||
|
||
<div class="function-monitor-content"> | ||
<div *ngIf = "!shouldRenderMonitorConfigure()" class="function-monitor-content"> | ||
|
||
<div class="alert alert-warning alert-dismissible" role="alert"> | ||
<img src="image/appInsights.svg"/> | ||
<span [innerHTML]="'monitoring_appInsights' | translate"></span> |
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 know this line isn't your code but can we please make this {{'monitoring_appInsights' | translate}}
innerHtml for this seems really wierd #Resolved
</command-bar> | ||
|
||
<app-card-info-control | ||
[image]="'image/info.svg'" |
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.
nit: image="image/info.svg" (I know this is my favorite copy/paste mistake in my own code 😛 ) #Resolved
@@ -0,0 +1 @@ | |||
@import '../../../sass/common/variables'; |
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.
remove? #Resolved
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.
Yeah, initially I thought I might have some custom styles.. but doesn't look like it. Will leave the file, just in case if this changes. #Resolved
@@ -1028,4 +1029,8 @@ | |||
public static deploymentCenterTitle: string = "deploymentCenterTitle"; | |||
public static deploymentCenterDesc: string = "deploymentCenterDesc"; | |||
public static monitorHostFetchFailed: string = "monitorHostFetchFailed"; | |||
public static configureAppInsightsTitle: string = "configureAppInsightsTitle"; |
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 just realized I should remove :string from this file. Any other place that would cause linting issues #Resolved
} | ||
}); | ||
const resultJson = response.result.json(); | ||
if (!!resultJson) { |
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.
maybe some commenting on what is going on here? What are element[0] and element[1]? #Resolved
import { ARMApplicationInsightsDescriptior } from "../resourceDescriptors"; | ||
import { ErrorEvent } from "./error-event"; | ||
import { FunctionAppContext } from '../function-app-context'; | ||
import { Dictionary } from 'lodash'; |
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.
please dont import all of lodash, it's huge. I dont think Dictionary is available yet as an es6 module though. Can you use { [key:string] : string} instead? #Resolved
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.
return monitorConfigureInfoInputEvent | ||
.switchMap(monitorConfigureInfo => Observable.zip( | ||
Observable.of(monitorConfigureInfo), | ||
)) |
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 switchMap isn't doing anything and you can remove it #Resolved
You don't have the HTML file for this component in your PR, but from your video I saw the red/green text for error and successful requests. Would it be possible to show a green checkmark icon for success, and a red error icon for failure, and then make the text black? I think it may get called out by accessibility. #Resolved Refers to: client/src/app/function-monitor/monitor-applicationinsights/monitor-applicationinsights.component.ts:18 in 3668d7e. [](commit_id = 3668d7e, deletion_comment = False) |
summary.failedCount = element[1]; | ||
} | ||
}); | ||
const resultJson = response.result.json(); |
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.
response.result.json() [](start = 25, length = 22)
Is there a type for this? #Pending
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.
Are you referring to creating a type to represent this? #Resolved
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.
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.
Sorry yes. I meant that there should be a type for this instead of any
In reply to: 182514236 [](ancestors = 182514236,182266726)
} | ||
} | ||
|
||
public setFunctionMonitorClassicViewPreference(functionAppResourceId: string, value: string) { |
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.
public setFunctionMonitorClassicViewPreference(functionAppResourceId: string, value: string) { [](start = 4, length = 94)
Can you add these to the application-insights.service? This service should mainly only be used for handling communication to local storage. #Resolved
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.
good point. #Resolved
@@ -170,7 +174,8 @@ export class BroadcastService { | |||
case BroadcastEvent.ClearError: | |||
return this.clearErrorEvent; | |||
|
|||
|
|||
case BroadcastEvent.RefreshMonitoringView: | |||
return this.refreshMonitorEvent; |
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 isn't required if you're using broadcastEvent and getEvent methods. #Resolved
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 have created a separate issue to just go through all the new experience and make sure accessiblity is properly addressed. I will add this to that issue and send out a separate PR. In reply to: 382182873 [](ancestors = 382182873) Refers to: client/src/app/function-monitor/monitor-applicationinsights/monitor-applicationinsights.component.ts:18 in 3668d7e. [](commit_id = 3668d7e, deletion_comment = False) |
https://app.zenhub.com/workspace/o/azure/azure-functions-ux/issues/2574 In reply to: 382472917 [](ancestors = 382472917,382182873) Refers to: client/src/app/function-monitor/monitor-applicationinsights/monitor-applicationinsights.component.ts:18 in 3668d7e. [](commit_id = 3668d7e, deletion_comment = False) |
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.
* Introduce application insights based monitoring view (#2516) Introduce application insights based monitoring view and do a little of refactor on the existing monitoring view. * Redoing the extension cache fix (#2555) * [SpecPicker] Adding individual banner to each group. Also several other bug fixes (#2552) * Removing AzureFunctions.AngularClient folder * Pass context for create (#2559) * Initial upload experience for Prod Functions (#2557) * Hotfix for set model after token loads (#2562) (#2563) * Hotfix for set model after token loads * update v1 version number * adding top level tslint for codacy * adjusting linting rules for codacy * adding azure-cli script for swapping ux slots (#2568) * add control with placeholder logic * add lodash to blacklisted imports (#2572) * update setting values * adding moment to blacklist as well (#2573) * updating production slot swap to fix a couple of bugs * update resources * Add metrics feature to monitoring section (#2578) Add metrics feature link to monitoring page. * Fixes #2480 - Add accessibility support to spec picker (#2575) * Fixes #2480 - Add accessibility to spec picker * Updated from PR comments * Add ~2 to list of stable runtimes. Closes #2564 * Set readOnly state for prod dynmic linux. Closes #2565 * Configure page for Application Insights (#2560) monitor configure page * update check scenario * Fix slot locks * Update name of site config property * poll function logs if the app is dynamic linux. Closes #2571 * Update id check (#2588) * PR updates * Wait on existing polling request if it takes longer than 2 seconds. * more pr updates * remove console log * Add separate list for recommended skus and add resources for strings (#2585) * Add separate list for recommended skus and add resources for strings * Removing duplicate identifier * Updated from feedback * updated feedback * hide behind feature flag * Inject build number to package.json and an api * Collect perf counters from backend * Add build defintion * as per discussion with elliot prepending 1.0. to the build number for version number * fixing issue where server won't run locally due to invalid package version * make server file pull version from package.json * update axios to 0.18 * set keepAlive to true for proxied connections * Fix issue where UI doesn't handle scaling an isolated SKU. Plus other bug fixes (#2599) * Fix issue where UI doesn't handle scaling an isolated SKU. Plus other bug fixes * Remove shield but disable apply button when isolated scale operation in progress * Updated from PR feedback * Updated links and changed wording on ACU language * Add global mocks for log and telemetry service (#2601) Add global mocks for log and telemetry service * Fix initial items Alex found. (#2598) * prefix class name (#2602) * Add dark theme to spec picker. Closes #2481 (#2604) * Removing feature flag and enabling new spec picker by default (#2605) * Removing feature flag and enabling new spec picker by default * remove url import * CICD polish (#2609) * Changing auth token to be passed through body of passthrough rather t… (#2618) * Changing auth token to be passed through body of passthrough rather than header * Codacy * Update the scenario status for all environments for AI based monitoring. (#2615) Update the scenario status for all environments for AI based monitoring. * Spec picker bugs for styling and xenon (#2630) * Spec picker bugs for styling and xenon * PR feedback * More styling, fwdlinks, and spec clean up - Fixes #2627, Fixes #2612, Fixes #2628 (#2632) * Add support for linux pricing (#2635) * Fix up some acessibility items and dark theme items. (#2634) * Inject empty headers to urls going to our backend, not using arm headers (#2636) * Inject empty headers to urls going to our backend, not using arm headers * Codacy * Add busy state when opening create sidebar (#2633) * add busy state when opening create sidebar * use busy manager * remove underscore * Handle busy error (#2637) * Changing desgin of cards for source and build providers (#2639) * Changing desgin of cards for source and build providers * udpating strings * PR Comments * changing settings to not show runtime info if compose or kubernetes i… (#2640) * changing settings to not show runtime info if compose or kubernetes is used * PR Comment * KUBE| * Make the datetime more granular and add sorting capability. (#2647) * Adding diagnose and solve problems (#2641) * Adding diagnose and solve problems * Removing RBAC check * Sort app settings by name (#2648) * Use resources for FTP/S options (#2650) * Fix the table styling, delay message, and linux function configuration page (#2649) Fix the table styling, delay message, and linux function configuration page. * Update extension install message (#2651) * update extension install message * + is * Add a close button on top right. This aligns with some of the other slide outs with the close buttons. (#2652) * Add a close button on top right. This aligns with some of the other slide outs with the close buttons. * Change to Subject() * Move application insights link to essentials col (#2653) * Move application insights link to essentials col Indicate number of results returned in query and provide a link to open in the AI query editor. * PR Comments. * Fixing issue where S2 and S3 are not available for Linux (#2655) * Fixing issue where S2 and S3 are not available for Linux * Removing kinds reference * String change for AI configuration instructions. (#2656) * Remove FTP feature flag (#2657) * Hardcoding Linux Pv2 regions until the georegions API works properly (#2658) * Fix read\write -> read/write. Closes #2507 (#2659) * Adding discount banner for existing linux isolated group in spec picker (#2660) * Updating MSI title to preview * Updating resource strings
Adding a configure page for Application Insights. The users can temporarily opt out of it by switching to classic view, but that setting is stored locally. The goal is to drive all users to using application insights for their logs.
Video of the scenario : https://microsoft-my.sharepoint.com/:v:/p/michinoy/ESYr0w2rXH9PjNFvePFfxkoBokcd9_u3X_vakR0sFnJs0Q?e=onlHLq
There is a delay (3 - 5 mins) in app insights getting the data: