[MetricsAdvisor] Overwriting patch serialization methods to support sending null #22457

kinelski · 2021-07-06T08:17:37Z

Part of #21509.

The service allows sending null during an Update operation to reset a property to its default value. So an Update has the three possible scenarios for each property:

Send a value: the value will be updated.
Do not send a value: the value will be left as it is.
Send null: the property will be reset to its default value.

CodeGen does not support this differentiation. Currently, our code only supports the first two scenarios. If a property is null, we'll simply not send it. This prevents users from resetting settings to default but, most important, it prevents users from removing configurations. For example, we have the following class in one of our configuration APIs:

public class MetricWholeSeriesDetectionCondition
{
   public SmartDetectionCondition SmartDetectionCondition { get; set; }

   public HardThresholdCondition HardThresholdCondition { get; set; }
}

Each property adds a new behavior to the configuration. You need to specify at least one of them, and both can be set for the same config. Let's say the customer does not want to use the HardThresholdCondition anymore. Naturally, they would do:

var config = client.GetConfig(...);

config.WholeSeriesDetectionCondition.HardThresholdCondition = null;
client.UpdateConfig(config);

However, since CodeGen does not send the property when it's null, we're not sending anything to the service, and they have no way of knowing we want to remove this behavior.

This PR fixes this behavior for Data Feeds only (next PRs will contain other classes). Implementation details explained in the comments.

kinelski · 2021-07-06T08:21:55Z

...I.MetricsAdvisor/src/Generated/Models/AzureApplicationInsightsDataFeedPatch.Serialization.cs

-            if (Optional.IsDefined(DataFeedName))
-            {
-                writer.WritePropertyName("dataFeedName");
-                writer.WriteStringValue(DataFeedName);
-            }


As you can see, the serialization method does not send null to the service. Unfortunately, we need to overwrite all serialization methods for Patch models (used during Update operations only) in order to send null when a property is not present.

kinelski · 2021-07-06T08:22:41Z

sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/Models/CodeGenInternal/DataFeedDetailPatch.cs

-
-namespace Azure.AI.MetricsAdvisor.Models
-{
-    internal partial class DataFeedDetailPatch : IUtf8JsonSerializable


Moved this to another folder (Models/CodeGenInternal/PatchModels)

kinelski · 2021-07-06T08:25:03Z

...tricsAdvisor/src/Models/CodeGenInternal/PatchModels/AzureApplicationInsightsDataFeedPatch.cs

+                writer.WritePropertyName("dataSourceParameter");
+                writer.WriteObjectValue(DataSourceParameter);
+            }
+            SerializeCommonProperties(writer);


All <...>DataFeedPatch models, including this one, are subclasses of DataFeedDetailPatch. I put serialization of all properties of the base class in SerializeCommonProperties to avoid repeating (a lot of) code, specially since we have 13 subtypes.

kinelski · 2021-07-06T08:28:59Z

...ricsAdvisor/src/Models/CodeGenInternal/PatchModels/AzureApplicationInsightsParameterPatch.cs

+            if (Optional.IsDefined(ApiKey))
+            {
+                writer.WritePropertyName("apiKey");
+                writer.WriteStringValue(ApiKey);
+            }


We're not sending null for secrets, though. There's a good reason for this. The service does not return secrets to us, so this would happen:

var dataFeed = client.GetDataFeed(...); // Here, dataFeed.DataSource.ApiKey is null because it was not returned by the service. // Update some properties here // This would send apiKey = null to the service, even though we didn't touch it. client.UpdateDataFeed(dataFeed);

and will that cause an error in the service? if this was allowed before, would it be better to add an entry to the changelog saying how this "bug" got fixed?

and will that cause an error in the service?

Correct. We get back an error message saying the secret is required.

if this was allowed before, would it be better to add an entry to the changelog saying how this "bug" got fixed?

We were not overwriting the serialization method before, so we were not sending null. My point in this comment is "let's keep the ApiKey behaving as it already was. If we change it, it breaks".

kinelski · 2021-07-06T08:33:57Z

sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/Utf8JsonWriterExtensions.cs

+
+namespace Azure.AI.MetricsAdvisor
+{
+    internal static class Utf8JsonWriterExtensions


Convenience methods to make serialization code shorter. We can reduce:

if (value != null) { writer.WritePropertyName(...); writer.Write<Something>Value(...); } else { writer.WriteNull(propertyName); }

to a single line.

kinelski · 2021-07-06T08:34:52Z

...dvisor/Azure.AI.MetricsAdvisor/tests/MetricsAdvisorAdministrationClient/DataFeedLiveTests.cs

+
+            string dataFeedName = Recording.GenerateAlphaNumericId("dataFeed");
+            DataFeedSource dataSource = CreateMockDataFeedSource(dataSourceKind);
+            var dataFeedToCreate = new DataFeed()


Set initial values for required properties and for anything that can be updated.

kinelski · 2021-07-06T08:35:20Z

...dvisor/Azure.AI.MetricsAdvisor/tests/MetricsAdvisorAdministrationClient/DataFeedLiveTests.cs

+            dataFeedToUpdate.IngestionSettings.IngestionStartOffset = null;
+            dataFeedToUpdate.IngestionSettings.IngestionRetryDelay = null;
+            dataFeedToUpdate.IngestionSettings.StopRetryAfter = null;
+            dataFeedToUpdate.IngestionSettings.DataSourceRequestConcurrency = null;
+            dataFeedToUpdate.Schema.TimestampColumn = null;
+            dataFeedToUpdate.Description = null;
+            dataFeedToUpdate.RollupSettings.RollupType = null;
+            dataFeedToUpdate.RollupSettings.AutoRollupMethod = null;
+            dataFeedToUpdate.RollupSettings.RollupIdentificationValue = null;
+            dataFeedToUpdate.MissingDataPointFillSettings = null;
+            dataFeedToUpdate.AccessMode = null;
+            dataFeedToUpdate.ActionLinkTemplate = null;


Set everything that can be updated to null to restore the default values.

kinelski · 2021-07-06T08:35:41Z

...dvisor/Azure.AI.MetricsAdvisor/tests/MetricsAdvisorAdministrationClient/DataFeedLiveTests.cs

+            Assert.That(updatedDataFeed.IngestionSettings.IngestionStartOffset, Is.EqualTo(TimeSpan.Zero));
+            Assert.That(updatedDataFeed.IngestionSettings.IngestionRetryDelay, Is.EqualTo(TimeSpan.FromSeconds(-1)));
+            Assert.That(updatedDataFeed.IngestionSettings.StopRetryAfter, Is.EqualTo(TimeSpan.FromSeconds(-1)));
+            Assert.That(updatedDataFeed.IngestionSettings.DataSourceRequestConcurrency, Is.EqualTo(-1));
+            Assert.That(updatedDataFeed.Schema.TimestampColumn, Is.Empty);
+            Assert.That(updatedDataFeed.Description, Is.Empty);
+            Assert.That(updatedDataFeed.RollupSettings.RollupType, Is.EqualTo(DataFeedRollupType.NoRollupNeeded));
+            Assert.That(updatedDataFeed.RollupSettings.AutoRollupMethod, Is.EqualTo(DataFeedAutoRollupMethod.None));
+            Assert.That(updatedDataFeed.RollupSettings.RollupIdentificationValue, Is.Null);
+            Assert.That(updatedDataFeed.MissingDataPointFillSettings.FillType, Is.EqualTo(DataFeedMissingDataPointFillType.SmartFilling));
+            Assert.That(updatedDataFeed.AccessMode, Is.EqualTo(DataFeedAccessMode.Private));
+            Assert.That(updatedDataFeed.ActionLinkTemplate, Is.Empty);


Check if default values are correct.

kinelski · 2021-07-06T08:36:27Z

...dvisor/Azure.AI.MetricsAdvisor/tests/MetricsAdvisorAdministrationClient/DataFeedLiveTests.cs

+        }
+
+        [RecordedTest]
+        public async Task UpdateAzureBlobDataFeedAuthenticationWithNullSetsToDefault()


The following tests handle properties specific to each data feed source. The test above was for common properties only.

check-enforcer · 2021-07-06T08:53:00Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them. In order to bootstrap pipelines for a new service, please perform following steps:

For data-plane/track 2 SDKs Issue the following command as a pull request comment:

/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run net - [service] - ci

For track 1 management-plane SDKs

Please open a separate PR and to your service SDK path in this file. Once that PR has been merged, you can re-run the pipeline to trigger the verification.

maririos

Looks like a changelog entry and some samples will be good to showcase this new feature to the user

maririos · 2021-07-06T18:31:04Z

...ricsAdvisor/src/Models/CodeGenInternal/PatchModels/AzureApplicationInsightsParameterPatch.cs

+            if (Optional.IsDefined(ApiKey))
+            {
+                writer.WritePropertyName("apiKey");
+                writer.WriteStringValue(ApiKey);
+            }


and will that cause an error in the service? if this was allowed before, would it be better to add an entry to the changelog saying how this "bug" got fixed?

maririos · 2021-07-06T18:52:31Z

sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/Utf8JsonWriterExtensions.cs

+{
+    internal static class Utf8JsonWriterExtensions
+    {
+        public static void WriteNullStringValue(this Utf8JsonWriter writer, string propertyName, string value)


I wonder if to simplify all this code, you could do something like this https://github.com/Azure/autorest.csharp/blob/f790582c450f6f90e173a12e96c8e83a745a0b0f/src/assets/Generator.Shared/Utf8JsonWriterExtensions.cs#L52

I really liked the suggestion. Created an issue to track it (#22472). Won't update the code now since it's only an internal change and it may affect 4 open PRs right now.

kinelski · 2021-07-06T20:18:11Z

Looks like a changelog entry and some samples will be good to showcase this new feature to the user

Added a changelog entry in the New Features section to highlight it. Wasn't sure where to put it, since technically it's a breaking change. Setting a property to null in an Update operation before this change would make literally no change in the service, so I don't expect to have customers actually affected by this.

Regarding samples, I made a small addition to the UpdateDataFeedAsync code sample we have after your suggestion:

// Some properties, such as IngestionStartOffset, can be reset to their default value
// when set to null during an Update operation. Check the API documentation to verify
// when a property supports this feature.

dataFeed.IngestionSettings.IngestionStartOffset = null;

response = await adminClient.UpdateDataFeedAsync(dataFeed);
// ... Print new value

Do you think it may be worth adding more properties to the sample?

I'm going through all docstrings in the library and fixing anything that sounds off or poor written. I'm also adding remarks pointing out when a property can be reset to its default value during an Update operation.

The Update scenario is not one of our core scenarios for this library, so this is not a big change from the customers' point of view (even though it gave us so much trouble in terms of code). Update and management operations in general usually are a single-time event, so we expect users to do this through the portal more often.

maririos · 2021-07-06T20:53:39Z

Do you think it may be worth adding more properties to the sample?

Not all of them but make sure that in the more advanced samples this is showcased.
The addition to the docstrings I think is more important so hopefully with that user can figure out this behavior.

If, after release, we hear from the community that they need more documentation, then you can add more specific samples

Networking 2022-09-01 release (Azure#22639) * Adds base for updating Microsoft.Network from version stable/2022-07-01 to version 2022-09-01 * Updates readme * Updates API version in new specs and examples * Added flowlog property in virtual network (Azure#21790) Co-authored-by: Krishna Mishra <krmishr@microsoft.com> * commit1 (Azure#22111) Co-authored-by: Khushboo Baheti <khbaheti@microsoft.com> * adding auth status property to circuit (Azure#22024) * Make auth status readonly (Azure#22365) * make auth status read only * fixing model validation * prettier fix * Add support for State flag in Custom Rule (Azure#22457) * Fix LRO header model validation (Azure#22506) * Add new status code for application gateway custom error page (Azure#22151) * Add new status code for application gateway custom error page * Fix prettier * Adding words to Custom-Words list * Fix missing resource id in application gateway list example (Azure#22509) * Resolving merge conflicts with main branch --------- Co-authored-by: Mikhail <mitryakh@microsoft.com> Co-authored-by: KRISHNA MISHRA <krishmi139@gmail.com> Co-authored-by: Krishna Mishra <krmishr@microsoft.com> Co-authored-by: Khushboo Baheti <37917868+Khushboo-Baheti@users.noreply.github.com> Co-authored-by: Khushboo Baheti <khbaheti@microsoft.com> Co-authored-by: utbarn-ms <66377251+utbarn-ms@users.noreply.github.com> Co-authored-by: tejasshah7 <49326906+tejasshah7@users.noreply.github.com> Co-authored-by: Prateek Sachan <42961174+prateek2211@users.noreply.github.com>

kinelski added 5 commits July 5, 2021 19:21

Updated data feed tests

7e0c2f4

Merge branch 'camaiaor/ma-beta5' into ma-patch

763c1d5

Updated patch models for datafeed-related models

35ca9cf

Finished data feed implementation

188fffc

Updated recordings

fcca5d9

kinelski added the Client This issue points to a problem in the data-plane of the library. label Jul 6, 2021

kinelski self-assigned this Jul 6, 2021

ghost added the Cognitive - Metrics Advisor label Jul 6, 2021

kinelski commented Jul 6, 2021

View reviewed changes

Updated serialization

46873ce

kinelski requested a review from maririos July 6, 2021 08:42

maririos reviewed Jul 6, 2021

View reviewed changes

Addressed comments

800dbe4

maririos approved these changes Jul 6, 2021

View reviewed changes

kinelski merged commit aa319b4 into Azure:camaiaor/ma-beta5 Jul 6, 2021

kinelski deleted the ma-patchDataFeed branch July 6, 2021 20:56

kinelski mentioned this pull request Jul 8, 2021

[MetricsAdvisor] Cannot overwrite serialization method in Patch models #21509

Closed

ibocon mentioned this pull request Sep 26, 2021

[MetricsAdvisor] Simplify Utf8JsonWriterExtensions and follow pattern. #24231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MetricsAdvisor] Overwriting patch serialization methods to support sending null #22457

[MetricsAdvisor] Overwriting patch serialization methods to support sending null #22457

kinelski commented Jul 6, 2021 •

edited

Loading

kinelski Jul 6, 2021

kinelski Jul 6, 2021 •

edited

Loading

kinelski Jul 6, 2021

kinelski Jul 6, 2021

maririos Jul 6, 2021

kinelski Jul 6, 2021

kinelski Jul 6, 2021

kinelski Jul 6, 2021

kinelski Jul 6, 2021 •

edited

Loading

kinelski Jul 6, 2021

kinelski Jul 6, 2021

check-enforcer bot commented Jul 6, 2021

maririos left a comment

maririos Jul 6, 2021

maririos Jul 6, 2021

kinelski Jul 6, 2021

kinelski commented Jul 6, 2021 •

edited

Loading

maririos commented Jul 6, 2021

[MetricsAdvisor] Overwriting patch serialization methods to support sending null #22457

[MetricsAdvisor] Overwriting patch serialization methods to support sending null #22457

Conversation

kinelski commented Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

kinelski Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinelski Jul 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

check-enforcer bot commented Jul 6, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

For data-plane/track 2 SDKs Issue the following command as a pull request comment:

For track 1 management-plane SDKs

maririos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinelski commented Jul 6, 2021 • edited Loading

maririos commented Jul 6, 2021

kinelski commented Jul 6, 2021 •

edited

Loading

kinelski Jul 6, 2021 •

edited

Loading

kinelski Jul 6, 2021 •

edited

Loading

kinelski commented Jul 6, 2021 •

edited

Loading