[MetricsAdvisor] Fixed UpdateAlertConfiguration methods #21514
Conversation
kinelski
added
Client
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/api/Azure.AI.MetricsAdvisor.netstandard2.0.cs
Show resolved
Hide resolved
...etricsAdvisor/tests/MetricsAdvisorAdministrationClient/AnomalyAlertConfigurationLiveTests.cs
Show resolved
Hide resolved
| @@ -11,6 +11,7 @@ | |||
|
|
|||
| namespace Azure.AI.MetricsAdvisor.Tests | |||
| { | |||
| [Ignore("Data is old and tests can't be recorded again")] | |||
There was a problem hiding this comment.
I know this is too suspicious, but the problem is that I can't record these tests again because the data we're trying to get from the service is not there anymore. My guess is that the service periodically deletes entries that are too old. I'll adjust the date times we use accordingly (and always use a relative time), but I'll do this in a separate PR to make this one easier to review.
There was a problem hiding this comment.
do you have an issue with that? consider linking the issue to this ignore message so it is easier to map
There was a problem hiding this comment.
I think I was too sleepy when I did this. The change is just updating some local IDs and rerecording some tests, so I think I can do it in this PR.
There was a problem hiding this comment.
In the end, the issue was caused because of a configuration we deleted by accident. Talked to the service team and they were able to restore the deleted config, so I removed the Ignore attribute.
| @@ -47,12 +47,12 @@ internal MetricAnomalyAlertConfiguration(string detectionConfigurationId, Metric | |||
| /// The identifier of the anomaly detection configuration to which this configuration applies. | |||
| /// </summary> | |||
| [CodeGenMember("AnomalyDetectionConfigurationId")] | |||
| public string DetectionConfigurationId { get; } | |||
| public string DetectionConfigurationId { get; set; } | |||
There was a problem hiding this comment.
if my assumption of codegen serializing with custom type is wrong then this is ok. If it is right, shouldn't there be a patch method for this property?
There was a problem hiding this comment.
I still haven't figured out what you were trying to say, but I think I explained the issue properly in the other PR.
In fact, the service doesn't have a patch version for MetricAnomalyAlertConfiguration. This type is not used directly in Create/Update methods. Actually, it is part of a collection (an array in the REST API):
// Type passed to the Create/Update methods
public class AnomalyAlertConfiguration
{
public IList<MetricAnomalyAlertConfiguration> AlertConfigurations { get; }
internal <model-name>Patch GetPatchModel()
{
...
}
}The service does not support a "partial" array update. According to Johan, this is what's expected according to the JSON "merge-patch" format, used by the MA service. So every time a user wants to update this array, all of its members must be specified in their entirety. That's why they didn't add a patch model for this type.
...etricsAdvisor/tests/MetricsAdvisorAdministrationClient/AnomalyAlertConfigurationLiveTests.cs
Show resolved
Hide resolved
| @@ -11,6 +11,7 @@ | |||
|
|
|||
| namespace Azure.AI.MetricsAdvisor.Tests | |||
| { | |||
| [Ignore("Data is old and tests can't be recorded again")] | |||
There was a problem hiding this comment.
do you have an issue with that? consider linking the issue to this ignore message so it is easier to map
Part of #21177.
Fixes #18457.