Populate feature flag telemetry metadata when feature flag telemetry is enabled #517

amerjusupovic · 2024-02-10T00:35:22Z

Adding 3 values to feature flag telemetry metadata when a flag has telemetry enabled: ETag, FeatureFlagId, and FeatureFlagReference.

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs

rossgrambo · 2024-02-12T18:49:59Z

...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs

+                        featureFlagIdHash = hashAlgorithm.ComputeHash(Encoding.UTF8.GetBytes($"{setting.Key}\n{setting.Label}"));
+                    }
+
+                    string featureFlagId = WebUtility.UrlEncode(Convert.ToBase64String(featureFlagIdHash));


This isn't quite accurate for what Jimmy purposed. He suggested Base64Url encoding- which is different than Base64 + Url encoding (see https://stackoverflow.com/a/55389212/5937135).

I see an AspNetCore method for it here: https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.webutilities.webencoders.base64urlencode?view=aspnetcore-8.0

You could achieve base64url encoding with

string base64UrlStr = Convert.ToBase64String(Encoding.UTF8.GetBytes(target)) .TrimEnd('=') // Remove padding (optional) .Replace('+', '-') // Replace '+' with '-' .Replace('/', '_'); // Replace '/' with '_'

But using a built-in method is probably preferred.

Oh I see, I think I misunderstood how the URL conversion works. I did see that AspNetCore method, but it's not compatible with the target frameworks in the main project, so I can't find another built in method that exists. I can change it to this for now.

rossgrambo · 2024-02-12T22:09:03Z

...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs

+
+                    using (HashAlgorithm hashAlgorithm = SHA256.Create())
+                    {
+                        featureFlagIdHash = hashAlgorithm.ComputeHash(Encoding.UTF8.GetBytes($"{setting.Key}\n{setting.Label}"));


I don't recall what the plan was- but if label is null- are we omitting the \n and null? I think currently this would give the string:

Key null

It should evaluate to "Key\n", but I'm not sure if we clarified whether we want to keep the newline character as part of the value even if there's a null label? I think it's fine as it is but just wanted to call it out.

Do we differentiate null vs. empty label?

I assumed we wouldn't, when I tried on portal I couldn't pass an empty string, and we also treat whitespace/null as the same for label filters in calls to Select.

rossgrambo

1 remaining comment but otherwise lgtm

jimmyca15 · 2024-02-13T22:37:15Z

...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs

+
+                    string featureFlagIdBase64 = Convert.ToBase64String(featureFlagIdHash);
+
+                    StringBuilder featureFlagIdBuilder = new StringBuilder(featureFlagIdBase64[featureFlagIdBase64.Length - 1] == '=' ? featureFlagIdBase64.Length - 1 : featureFlagIdBase64.Length);


Can't b64 have two equal signs at the end?

also, seems like you should be using IndexOf('=') and judge based off that.

Good catch. There's an appendix about implementing it we could follow https://datatracker.ietf.org/doc/html/rfc7515#appendix-C, they split to remove all "=", but index would work too (and probably better)

...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs

zhenlan · 2024-02-14T02:29:14Z

...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs

-                {
-                    keyValues.Add(new KeyValuePair<string, string>($"{telemetryPath}:{FeatureManagementConstants.Enabled}", telemetry.Enabled.ToString()));
-                }
-
                if (telemetry.Metadata != null)


Should we move this inside the if (telemetry.Enabled)? No one cares about it if telemetry is not enabled.

I see now, I thought there was a scenario in feature management where it could still be used but I don't see how. I can move it inside the enabled block too.

zhenlan · 2024-02-14T02:38:20Z

...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs

+                        {
+                            featureFlagIdBuilder.Append(featureFlagIdBase64[i]);
+                        }
+                    }


Can we add some comments to explain how the featureFlagId is supposed to be calculated? It's not clear why we are doing some of the operations we are doing here.

I think it would also add clarity if we move the b64url into an extension method.

ToBase64UrlString(this bytes[]);

At that point the code will be more self explanatory.

Right, won't mind if we move the whole featureFlagId calculation to a separate method. This function is getting way too long.

I think I'll go with the extension for now since it makes sense to me conceptually, but let me know if you have any other concerns

Will the extension method be reusable somewhere?

I just want to point out that the featureFlagId calculation is "by convention". It's not persisted anywhere. Portal and all our other providers will do the same calculation on-fly. Since the .NET provider is the first one doing this, it will make everyone's life easier if we can encapsulate it in one function with detailed comments so others can review/reference.

@amerjusupovic Can we please go with b64url extension method and private static method for CalculateFeatureFlagId(string key, string label)

sounds good to me

@zhenlan do you think the extension method is unnecessary? I thought it made sense to be an extension just because it's a known conversion, but it's true that we're only using it here.

I would recommend keeping that extension method. b64url is a spec and encapsulates well as utility method. Similar to frameworks System.Convert.ToBase64String

rossgrambo · 2024-02-14T18:10:28Z

...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs

+
+                    string featureFlagIdBase64 = Convert.ToBase64String(featureFlagIdHash);
+
+                    int indexOfEquals = featureFlagIdBase64.IndexOf("=");


Now that I think of it- the input to this function is consistent. We know the indexof will always be missing or the 44th character. We could just stop after 43 characters.

I think the indexOf makes it clearer what's happening so we can keep it for now. And if we ever change the underlying hash, indexOf would continue to work.

comment will be obsoleted by ToBase64Url extension method which takes unknown bytes as input.

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/BytesExtensions.cs

jimmyca15 · 2024-02-14T20:10:26Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/BytesExtensions.cs

+{
+    internal static class BytesExtensions
+    {
+        public static string ToBase64Url(this byte[] bytes)


Can you put a summary that mentions it converts byte array to b64 URL. Using the trim trailing = characters strategy and also link to b64 spec.

The link to the spec: https://datatracker.ietf.org/doc/html/rfc4648#section-5

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/BytesExtensions.cs

…xtensions/BytesExtensions.cs Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

zhenlan · 2024-02-15T00:03:39Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/BytesExtensions.cs

+            int indexOfEquals = bytesBase64.IndexOf("=");
+
+            // Remove all instances of "=" at the end of the string that were added as padding
+            int stringBuilderCapacity = indexOfEquals != -1 ? indexOfEquals : bytesBase64.Length;


This code trims everything after the first occurrence of the =. It doesn't match your comment. You assume the = is at the end of the string. I'm wondering why we don't simply use TrimEnd.

Base64 doesn't use equals as any value other than padding at the end, so it should work effectively the same. I had it as TrimEnd initially but I got lost with this other implementation that was suggested and I didn't reconsider it. I think it makes more sense to be TrimEnd to save iteration and be more clear. Jumped the gun on the merge, so I can make another PR.

I don't see why TrimEnd would be used, it's unnecessary allocation. Otherwise it would just be Convert.ToBase64String(bytes).Replace('+', '-').Replace('/', '_').TrimEnd('='). But that creates copies a bit carelessly.

Implementation is fine. The comment can be updated to clarify that we choose to skip the padding (if any) based on the spec.

zhenlan · 2024-02-15T00:15:38Z

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/BytesExtensions.cs

+
+            StringBuilder stringBuilder = new StringBuilder(stringBuilderCapacity);
+
+            // Construct Base64URL string by replacing characters in Base64 conversion that are not URL safe


Is there a reason the featureFlagId needs to be URL safe?

For use in REST resource IDs (/experiment/{id})

So, sounds like we should have a comment in CalculateFeatureFlagId to explain why we need base64url encoding.

That seems more like a design doc/specification territory rather than a code comment. The code just implements the spec of a feature flag id, which uses b64url encoding.

amerjusupovic added 7 commits February 2, 2024 14:24

design questions for Id format pending

429ec6e

first draft adding ffid and ffref

8ce6409

fix telemetry test bug

e068121

fix telemetry test and only populate when telemetry is enabled

9f5f126

give priority to provider populated metadata values

8ed359d

test label in telemetry test

1a95a53

keep metadata values together in output

5e36fe3

amerjusupovic requested review from jimmyca15 and rossgrambo February 10, 2024 00:35

rossgrambo reviewed Feb 12, 2024

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationProvider.cs Outdated Show resolved Hide resolved

remove unnecessary code for checking client manager endpoint

74553d8

rossgrambo reviewed Feb 12, 2024

View reviewed changes

fix base64url logic

d208f2e

rossgrambo reviewed Feb 12, 2024

View reviewed changes

rossgrambo approved these changes Feb 12, 2024

View reviewed changes

amerjusupovic added 2 commits February 12, 2024 15:54

check for whitespace in label

35e2741

use stringbuilder over replace and trim for performance

07f7c7f

jimmyca15 reviewed Feb 13, 2024

View reviewed changes

use indexof to check for equals

c166081

jimmyca15 reviewed Feb 14, 2024

View reviewed changes

...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs Outdated Show resolved Hide resolved

jimmyca15 approved these changes Feb 14, 2024

View reviewed changes

use static value for capacity in loop

d916ad4

zhenlan reviewed Feb 14, 2024

View reviewed changes

...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs Outdated Show resolved Hide resolved

zhenlan reviewed Feb 14, 2024

View reviewed changes

rossgrambo reviewed Feb 14, 2024

View reviewed changes

resolve comments

e7ba889

jimmyca15 reviewed Feb 14, 2024

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/BytesExtensions.cs Outdated Show resolved Hide resolved

amerjusupovic added 2 commits February 14, 2024 11:43

fix extension

c068492

pull out id calculation to private method

5815b5c

jimmyca15 reviewed Feb 14, 2024

View reviewed changes

jimmyca15 approved these changes Feb 14, 2024

View reviewed changes

amerjusupovic added 2 commits February 14, 2024 12:45

add summary comments

790e412

add base64 spec

a47f147

jimmyca15 reviewed Feb 14, 2024

View reviewed changes

src/Microsoft.Extensions.Configuration.AzureAppConfiguration/Extensions/BytesExtensions.cs Outdated Show resolved Hide resolved

jimmyca15 approved these changes Feb 14, 2024

View reviewed changes

Update src/Microsoft.Extensions.Configuration.AzureAppConfiguration/E…

80178fb

…xtensions/BytesExtensions.cs Co-authored-by: Jimmy Campbell <jimmyca@microsoft.com>

amerjusupovic merged commit 1d9ee4d into preview Feb 14, 2024
2 checks passed

amerjusupovic deleted the ajusupovic/populate-telemetry-metadata branch February 14, 2024 22:01

zhenlan reviewed Feb 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Populate feature flag telemetry metadata when feature flag telemetry is enabled #517

Populate feature flag telemetry metadata when feature flag telemetry is enabled #517

amerjusupovic commented Feb 10, 2024

rossgrambo Feb 12, 2024

amerjusupovic Feb 12, 2024

rossgrambo Feb 12, 2024

amerjusupovic Feb 12, 2024

zhenlan Feb 13, 2024

amerjusupovic Feb 13, 2024

rossgrambo left a comment

jimmyca15 Feb 13, 2024

rossgrambo Feb 13, 2024 •

edited

Loading

zhenlan Feb 14, 2024

amerjusupovic Feb 14, 2024

zhenlan Feb 14, 2024

jimmyca15 Feb 14, 2024

zhenlan Feb 14, 2024

amerjusupovic Feb 14, 2024

zhenlan Feb 14, 2024

zhenlan Feb 14, 2024

jimmyca15 Feb 14, 2024

amerjusupovic Feb 14, 2024

amerjusupovic Feb 14, 2024

jimmyca15 Feb 14, 2024 •

edited

Loading

rossgrambo Feb 14, 2024

jimmyca15 Feb 14, 2024

jimmyca15 Feb 14, 2024 •

edited

Loading

jimmyca15 Feb 14, 2024

zhenlan Feb 15, 2024

amerjusupovic Feb 15, 2024

jimmyca15 Feb 15, 2024

zhenlan Feb 15, 2024

zhenlan Feb 15, 2024

jimmyca15 Feb 15, 2024

zhenlan Feb 15, 2024

jimmyca15 Feb 15, 2024


		string featureFlagIdBase64 = Convert.ToBase64String(featureFlagIdHash);

		StringBuilder featureFlagIdBuilder = new StringBuilder(featureFlagIdBase64[featureFlagIdBase64.Length - 1] == '=' ? featureFlagIdBase64.Length - 1 : featureFlagIdBase64.Length);


		string featureFlagIdBase64 = Convert.ToBase64String(featureFlagIdHash);

		int indexOfEquals = featureFlagIdBase64.IndexOf("=");


		StringBuilder stringBuilder = new StringBuilder(stringBuilderCapacity);

		// Construct Base64URL string by replacing characters in Base64 conversion that are not URL safe

Populate feature flag telemetry metadata when feature flag telemetry is enabled #517

Populate feature flag telemetry metadata when feature flag telemetry is enabled #517

Conversation

amerjusupovic commented Feb 10, 2024

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

rossgrambo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossgrambo Feb 13, 2024 • 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

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

jimmyca15 Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmyca15 Feb 14, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossgrambo Feb 13, 2024 •

edited

Loading

jimmyca15 Feb 14, 2024 •

edited

Loading

jimmyca15 Feb 14, 2024 •

edited

Loading