[MetricsAdvisor] Remove unnecessary ClientOptions type #16436
Conversation
kinelski
added
Client
| @@ -8,17 +8,21 @@ | |||
| - Added a public setter to `DataFeed.Options`. | |||
|
|
|||
| ### Breaking Changes | |||
| - In `MetricsAdvisorClient`, updated `CreateDataFeed` and `CreateDataFeedAsync` to take a whole `DataFeed` object as a parameter. | |||
There was a problem hiding this comment.
Fixed it. It should be MetricsAdvisorAdministrationClient.
| - Renamed `MetricDimension` to `DataFeedDimension`. | ||
| - Renamed `DataAnomaly` to `DataPointAnomaly`. | ||
| - Renamed `IncidentStatus` to `AnomalyIncidentStatus`. | ||
| - Renamed `AlertingHook`, `EmailHook`, and `WebHook` to `NotificationHook`, `EmailNotificationHook`, and `WebNotificationHook`, respectively. | ||
| - Renamed `TimeMode` to `AlertQueryTimeMode`. | ||
|
|
||
| ### Key Bug Fixes | ||
| - Fixed a bug in `GetMetricEnrichedSeriesData` sync and async methods where a `NullReferenceException` was thrown if a returned data point had missing data. |
There was a problem hiding this comment.
Missed this in an older PR. Fixed by:
In
MetricEnrichedSeriesData, made elements ofExpectedValues,Periods,IsAnomaly,LowerBoundariesandUpperBoundariesnullables.
| <NoWarn> | ||
| $(NoWarn); | ||
| AZC0007; <!-- https://github.com/Azure/azure-sdk-for-net/issues/16435 --> | ||
| </NoWarn> |
There was a problem hiding this comment.
Blocked by Azure/azure-sdk-tools#127 (comment).
I tried suppressing the warning locally, but it still fails when exporting the API.
kinelski
requested review from
christothes and
maririos
and removed request for
tg-msft and
KrzysztofCwalina
| @@ -4,7 +4,7 @@ public partial class MetricsAdvisorClient | |||
| { | |||
| protected MetricsAdvisorClient() { } | |||
| public MetricsAdvisorClient(System.Uri endpoint, Azure.AI.MetricsAdvisor.MetricsAdvisorKeyCredential credential) { } | |||
| public MetricsAdvisorClient(System.Uri endpoint, Azure.AI.MetricsAdvisor.MetricsAdvisorKeyCredential credential, Azure.AI.MetricsAdvisor.MetricsAdvisorClientOptions options) { } | |||
| public MetricsAdvisorClient(System.Uri endpoint, Azure.AI.MetricsAdvisor.MetricsAdvisorKeyCredential credential, Azure.AI.MetricsAdvisor.MetricsAdvisorClientsOptions options) { } | |||
There was a problem hiding this comment.
The plural Clients seems odd to me. Should we just keep the name the same and delete the other one?
There was a problem hiding this comment.
Oh, I see in the linked issue that this is the guideline 😞
There was a problem hiding this comment.
omg I did the same. I was like.. ugh that name looks weird.... looked at guidelines.. ohh nooo :(.
Oh well, sad approve
Fixes #16369.
We currently have:
These were defined separately for both of our clients, but the service does not differentiate between admin/other operations. We can merge them into the same type: MetricsAdvisorClientsOptions.