Skip to content
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.

Sign up for GitHub

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

[MetricsAdvisor] Fix Update methods issues #16696

Merged
merged 3 commits into from
Nov 9, 2020
Merged

[MetricsAdvisor] Fix Update methods issues #16696

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1121917
Updated changelog
kinelski Nov 6, 2020
1024840
Fixed default issue in other parts of the code
kinelski Nov 6, 2020
63f7459
Added recordings
kinelski Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions sdk/metricsadvisor/Azure.AI.MetricsAdvisor/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@

### Key Bug Fixes
- Fixed a bug in sync and async `GetMetricEnrichedSeriesData` methods where a `NullReferenceException` was thrown if a returned data point had missing data.
- Fixed a bug in sync and async `UpdateDataFeed` methods where a `RequestFailedException` was thrown if a data feed without custom `DataFeedMissingDataPointFillType` was updated.
- Fixed a bug in sync and async `UpdateAlertConfiguration` methods where a `RequestFailedException` was thrown if a configuration with only one `MetricAnomalyAlertConfiguration` was updated.

## 1.0.0-beta.1 (2020-10-08)

Expand Down
2 changes: 1 addition & 1 deletion ...csadvisor/Azure.AI.MetricsAdvisor/src/Models/AlertTriggering/AnomalyAlertConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ internal AnomalyAlertingConfigurationPatch GetPatchModel()
{
return new AnomalyAlertingConfigurationPatch()
{
CrossMetricsOperator = CrossMetricsOperator.HasValue ? new AnomalyAlertingConfigurationPatchCrossMetricsOperator(CrossMetricsOperator.Value.ToString()) : default,
CrossMetricsOperator = CrossMetricsOperator.HasValue ? new AnomalyAlertingConfigurationPatchCrossMetricsOperator(CrossMetricsOperator.Value.ToString()) : default(AnomalyAlertingConfigurationPatchCrossMetricsOperator?),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the source of the issue in UpdateAlertConfiguration. CrossMetricsOperator is a nullable struct, and we were mistakenly expecting default to be null.

To make it easier to illustrate, let's use a clearer example:

bool condition = ...;
int? a = condition ? 10 : default;

If condition == true, we'll have a = 10. If condition == false, we'll have a = 0 (instead of null).

The ternary operator is evaluated before the attribution, and the compiler guesses the result must be an int, so it makes default(int) instead of default(int?). For this reason, we need to make it clear:

int? a = condition ? 10 : default(int?);

I have updated other parts of the code in which we had the same issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting bug - Seems like it is defined in this issue dotnet/csharplang#33

Description = Description,
Name = Name,
HookIds = IdsOfHooksToAlert.Select(h => new Guid(h)).ToList(),
Expand Down
10 changes: 5 additions & 5 deletions sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/Models/DataFeed/DataFeed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ internal DataFeedDetailPatch GetPatchModel()
DataFeedDetailPatch patch = DataSource.InstantiateDataFeedDetailPatch();

patch.DataFeedName = Name;
patch.Status = Status.HasValue ? new DataFeedDetailPatchStatus(Status.ToString()) : default;
patch.Status = Status.HasValue ? new DataFeedDetailPatchStatus(Status.ToString()) : default(DataFeedDetailPatchStatus?);

patch.TimestampColumn = Schema.TimestampColumn;

Expand All @@ -256,19 +256,19 @@ internal DataFeedDetailPatch GetPatchModel()

patch.DataFeedDescription = Description;
patch.ActionLinkTemplate = ActionLinkTemplate;
patch.ViewMode = AccessMode.HasValue == true ? new DataFeedDetailPatchViewMode(AccessMode.ToString()) : default;
patch.ViewMode = AccessMode.HasValue == true ? new DataFeedDetailPatchViewMode(AccessMode.ToString()) : default(DataFeedDetailPatchViewMode?);

if (RollupSettings != null)
{
patch.AllUpIdentification = RollupSettings.AlreadyRollupIdentificationValue;
patch.NeedRollup = RollupSettings.RollupType.HasValue ? new DataFeedDetailPatchNeedRollup(RollupSettings.RollupType.ToString()) : default;
patch.RollUpMethod = RollupSettings.RollupMethod.HasValue ? new DataFeedDetailPatchRollUpMethod(RollupSettings.RollupMethod.ToString()) : default;
patch.NeedRollup = RollupSettings.RollupType.HasValue ? new DataFeedDetailPatchNeedRollup(RollupSettings.RollupType.ToString()) : default(DataFeedDetailPatchNeedRollup?);
patch.RollUpMethod = RollupSettings.RollupMethod.HasValue ? new DataFeedDetailPatchRollUpMethod(RollupSettings.RollupMethod.ToString()) : default(DataFeedDetailPatchRollUpMethod?);
patch.RollUpColumns = RollupSettings.AutoRollupGroupByColumnNames;
}

if (MissingDataPointFillSettings != null)
{
patch.FillMissingPointType = MissingDataPointFillSettings.FillType.HasValue ? new DataFeedDetailPatchFillMissingPointType(MissingDataPointFillSettings.FillType.ToString()) : default;
patch.FillMissingPointType = MissingDataPointFillSettings.FillType.HasValue ? new DataFeedDetailPatchFillMissingPointType(MissingDataPointFillSettings.FillType.ToString()) : default(DataFeedDetailPatchFillMissingPointType?);
patch.FillMissingPointValue = MissingDataPointFillSettings.CustomFillValue;
}

Expand Down
4 changes: 3 additions & 1 deletion ...visor/Azure.AI.MetricsAdvisor/src/Models/DataFeed/DataFeedMissingDataPointFillSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ public DataFeedMissingDataPointFillSettings()
internal DataFeedMissingDataPointFillSettings(DataFeedDetail dataFeedDetail)
{
FillType = dataFeedDetail.FillMissingPointType;
CustomFillValue = dataFeedDetail.FillMissingPointValue;
CustomFillValue = dataFeedDetail.FillMissingPointType == DataFeedMissingDataPointFillType.CustomValue
? dataFeedDetail.FillMissingPointValue
: null;
Comment on lines -22 to +24
Copy link
Member Author

@kinelski kinelski Nov 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was the source of the issue in UpdateDataFeed. DataFeedMissingDataPointFillSettings has two properties.

One of them is FillMissingPointType (the strategy used to fill missing points in time series: NoFilling, PreviousValue, CustomValue, ...).

There's another property that must be specified when type is CustomValue: CustomFillValue, the const value used to fill the missing point.

When the fill type is NOT CustomValue, I'd expect the service to not return the CustomFillValue (or return null). However, it returns 0.0. When we call Get followed by Update, we store the 0 returned by the service and try to send it back (which is not allowed, because type is not CustomValue).

We're doing a workaround, but it may be something to fix on the service side.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#16701

}

/// <summary>
Expand Down
1 change: 0 additions & 1 deletion sdk/metricsadvisor/Azure.AI.MetricsAdvisor/tests/Samples/Sample01_DataFeedCrudOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ public async Task GetDataFeedAsync()
}

[Test]
[Ignore("https://github.com/Azure/azure-sdk-for-net/issues/16168")]
public async Task UpdateDataFeedAsync()
{
string endpoint = MetricsAdvisorUri;
Expand Down
1 change: 0 additions & 1 deletion ...dvisor/Azure.AI.MetricsAdvisor/tests/Samples/Sample05_AlertConfigurationCrudOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ public async Task GetAlertConfigurationAsync()
}

[Test]
[Ignore("https://github.com/Azure/azure-sdk-for-net/issues/16168")]
public async Task UpdateAlertConfigurationAsync()
{
string endpoint = MetricsAdvisorUri;
Expand Down