[Metrics Advisor] Initial merge commit #11593
Conversation
jeremymeng
requested review from
bterlson,
danieljurek,
mikeharder,
mitchdenny,
ramya-rao-a and
xirzec
as code owners
jeremymeng
force-pushed
the
feature/metricsadvisor
branch
from
f6d70b2
to
0972e7e
Compare
There was a problem hiding this comment.
I only had time to go through the README and the api review file, but I left some comments that can be mostly summarized by:
- A lot of identifiers are really long, it'd be nice to shorten things where we can.
- We have too many types already. We don't need to duplicate every data source to make one parameter optional.
- Some client methods have too many parameters.
Kinda wish there had been more time to review this before merging. I'm really not liking that aspect of doing work in the private repo, since it's hard to keep on top of those changes.
|
|
||
| const client = new MetricsAdvisorClient( | ||
| "<endpoint>", | ||
| new MetricsAdvisorKeyCredential("<subscription Key>", "<API key>") |
There was a problem hiding this comment.
should we re-use the same credential in the admin client?
|
|
||
| - timestamps | ||
| - zero or more dimensions | ||
| - one or more measures. |
There was a problem hiding this comment.
nit: this bullet has a period but none of the others do. I think you should remove it since these aren't full sentences.
| fillType: "SmartFilling" | ||
| }, | ||
| accessMode: "Private", | ||
| admins: ["xyz@microsoft.com"] |
There was a problem hiding this comment.
should this be @example.com since that's a guaranteed fake domain?
| console.log(`Alert configuration created: ${alertConfig.id}`); | ||
| } | ||
|
|
||
| async function configureAlertConfiguration(adminClient, detectionConfigId, hoookIds) { |
|
|
||
| const result = await iterator.next(); | ||
|
|
||
| if (!result.done) { |
There was a problem hiding this comment.
I understand that we wanted to show using the pageable without for/await, but this code feels pretty intense for showing you how to list alerts
There was a problem hiding this comment.
Agree. Changing to use for-await-of. We still have different ways of listing in the samples.
| export class MetricsAdvisorAdministrationClient { | ||
| constructor(endpointUrl: string, credential: MetricsAdvisorKeyCredential, options?: MetricsAdvisorAdministrationClientOptions); | ||
| createAnomalyAlertConfiguration(name: string, crossMetricsOperator: AnomalyAlertingConfigurationCrossMetricsOperator, metricAlertConfigurations: MetricAlertConfiguration[], hookIds?: string[], description?: string, options?: OperationOptions): Promise<GetAnomalyAlertConfigurationResponse>; | ||
| createDataFeed(name: string, source: DataFeedSource, granularity: DataFeedGranularity, schema: DataFeedSchema, ingestionSettings: DataFeedIngestionSettings, options?: CreateDataFeedOptions): Promise<GetDataFeedResponse>; |
There was a problem hiding this comment.
Similar as above.
| listDataFeedIngestionStatus(dataFeedId: string, startTime: Date, endTime: Date, options?: ListDataFeedIngestionStatusOptions): PagedAsyncIterableIterator<IngestionStatus, ListDataFeedIngestionStatusPageResponse>; | ||
| listDataFeeds(options?: ListDataFeedsOptions): PagedAsyncIterableIterator<DataFeed, ListDataFeedsPageResponse>; | ||
| listHooks(options?: ListHooksOptions): PagedAsyncIterableIterator<HookUnion, ListHooksPageResponse>; | ||
| listMetricAnomalyDetectionConfigurations(metricId: string, options?: OperationOptions): PagedAsyncIterableIterator<AnomalyDetectionConfiguration, ListAnomalyDetectionConfigurationsPageResponse, undefined>; |
There was a problem hiding this comment.
why is this MetricAnomaly but other methods refer to Anomaly without it being a MetricAnomaly - can we go for fewer words here?
There was a problem hiding this comment.
We probably wanted to emphasize that detection configuration is specific to a metric, so we have MetricAnomalyDetectionConfiguration but AnomalyAlertConfiguration
| deleteHook(hookId: string, options?: OperationOptions): Promise<RestResponse>; | ||
| deleteMetricAnomalyDetectionConfiguration(detectionConfigurationId: string, options?: OperationOptions): Promise<RestResponse>; | ||
| readonly endpointUrl: string; | ||
| getAnomalyAlertConfiguration(alertConfigurationId: string, options?: OperationOptions): Promise<GetAnomalyAlertConfigurationResponse>; |
There was a problem hiding this comment.
I wonder for all these get/delete methods if it's worth having such a verbose parameter name. Like wouldn't id be enough to name the thing? It's pretty clear from the method name what id it is and there is only one.
There was a problem hiding this comment.
id sounds good enough for get/update/delete. Changing
| // @public | ||
| export class MetricsAdvisorClient { | ||
| constructor(endpointUrl: string, credential: MetricsAdvisorKeyCredential, options?: MetricsAdvisorClientOptions); | ||
| createMetricFeedback(feedback: MetricFeedbackUnion, options?: {}): Promise<GetFeedbackResponse>; |
There was a problem hiding this comment.
shouldn't the options bag have a type? I assume it'd be at least OperationOptions
| listAlertsForAlertConfiguration(alertConfigurationId: string, startTime: Date, endTime: Date, timeMode: TimeMode, options?: ListAlertsOptions): PagedAsyncIterableIterator<Alert, ListAlertsForAlertConfigurationPageResponse>; | ||
| listAnomaliesForAlert(alertConfigurationId: string, alertId: string, options?: ListAnomaliesForAlertConfigurationOptions): PagedAsyncIterableIterator<Anomaly, ListAnomaliesForAlertPageResponse>; | ||
| listAnomaliesForDetectionConfiguration(detectionConfigurationId: string, startTime: Date, endTime: Date, options?: ListAnomaliesForDetectionConfigurationOptions): PagedAsyncIterableIterator<Anomaly, ListAnomaliesForDetectionConfigurationPageResponse>; | ||
| listDimensionValuesForDetectionConfiguration(detectionConfigurationId: string, startTime: Date, endTime: Date, dimensionName: string, options?: ListDimensionValuesForDetectionConfigurationOptions): PagedAsyncIterableIterator<string, ListDimensionValuesForDetectionConfigurationPageResponse>; |
There was a problem hiding this comment.
my eyes are starting to fuzz over from all these long identifier names, but I'm not sure how to shorten them
There was a problem hiding this comment.
Yeah I was following the pattern of MethodName + "Response". These are the page responses though so I feel it's better to also include "Page" in the name. I hope it's not too much an issue since they are output type and customers don't use these types a lot?
|
@xirzec Thanks! These are great feedback! I will try address them today and maybe some after this release. |
- Rename `*id` to just id for get/update/delete methods - also shorten `*ConfigurationId` to `*ConfigId` - Improve README - Swagger transform to add ref docs for `Metric` and `Dimension` - Swagger transform to remove `hook` prefix from `hookId` and `hookName` - Replace all `*DataFeedSourcePatch` types with a mapped type - Add missing DataFeed.creator property - Use mapped type for input of `create*()` methods instead of multiple ordered parameters. - Replace configuration patch types with mapped types from their full result types.
- Rename `*id` to just id for get/update/delete methods - also shorten `*ConfigurationId` to `*ConfigId` - Improve README - Swagger transform to add ref docs for `Metric` and `Dimension` - Swagger transform to remove `hook` prefix from `hookId` and `hookName` - Replace all `*DataFeedSourcePatch` types with a mapped type - Add missing DataFeed.creator property - Use mapped type for input of `create*()` methods instead of multiple ordered parameters. - Replace configuration patch types with mapped types from their full result types.
- Rename `*id` to just id for get/update/delete methods - also shorten `*ConfigurationId` to `*ConfigId` - Improve README - Swagger transform to add ref docs for `Metric` and `Dimension` - Swagger transform to remove `hook` prefix from `hookId` and `hookName` - Replace all `*DataFeedSourcePatch` types with a mapped type - Add missing DataFeed.creator property - Use mapped type for input of `create*()` methods instead of multiple ordered parameters. - Replace configuration patch types with mapped types from their full result types.
Fix s360 kpis for 2020-03-01. (Azure#11593) * Fix s360 kpis for 2020-03-01. * Delete Caches_Delete.json.bak Remove backup file that was accidentally pushed. * Delete Caches_Flush.json.bak Remove backup file that was accidentally pushed. * Delete Caches_Start.json.bak Remove backup file that was accidentally pushed. * Delete Caches_Stop.json.bak Remove backup file that was accidentally pushed. * Delete Caches_UpgradeFirmware.json.bak Remove backup file that was accidentally pushed. * Delete StorageTargets_Delete.json.bak Remove backup file that was accidentally pushed.
For the Metrics Advisor client library