-
Notifications
You must be signed in to change notification settings - Fork 33
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.
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
Populate feature flag telemetry metadata when feature flag telemetry is enabled #517
Changes from 9 commits
429ec6e
8ce6409
e068121
9f5f126
8ed359d
1a95a53
5e36fe3
74553d8
d208f2e
35e2741
07f7c7f
c166081
d916ad4
e7ba889
c068492
5815b5c
790e412
a47f147
80178fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,8 @@ | |
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Security.Cryptography; | ||
using System.Text; | ||
using System.Text.Json; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
|
@@ -20,7 +22,7 @@ public FeatureManagementKeyValueAdapter(FeatureFilterTracing featureFilterTracin | |
_featureFilterTracing = featureFilterTracing ?? throw new ArgumentNullException(nameof(featureFilterTracing)); | ||
} | ||
|
||
public Task<IEnumerable<KeyValuePair<string, string>>> ProcessKeyValue(ConfigurationSetting setting, Logger logger, CancellationToken cancellationToken) | ||
public Task<IEnumerable<KeyValuePair<string, string>>> ProcessKeyValue(ConfigurationSetting setting, Uri endpoint, Logger logger, CancellationToken cancellationToken) | ||
{ | ||
FeatureFlag featureFlag; | ||
try | ||
|
@@ -195,18 +197,41 @@ public FeatureManagementKeyValueAdapter(FeatureFilterTracing featureFilterTracin | |
|
||
string telemetryPath = $"{featureFlagPath}:{FeatureManagementConstants.Telemetry}"; | ||
|
||
if (telemetry.Enabled) | ||
{ | ||
keyValues.Add(new KeyValuePair<string, string>($"{telemetryPath}:{FeatureManagementConstants.Enabled}", telemetry.Enabled.ToString())); | ||
} | ||
|
||
if (telemetry.Metadata != null) | ||
{ | ||
foreach (KeyValuePair<string, string> kvp in telemetry.Metadata) | ||
{ | ||
keyValues.Add(new KeyValuePair<string, string>($"{telemetryPath}:{FeatureManagementConstants.Metadata}:{kvp.Key}", kvp.Value)); | ||
} | ||
} | ||
|
||
if (telemetry.Enabled) | ||
{ | ||
byte[] featureFlagIdHash; | ||
|
||
using (HashAlgorithm hashAlgorithm = SHA256.Create()) | ||
{ | ||
featureFlagIdHash = hashAlgorithm.ComputeHash(Encoding.UTF8.GetBytes($"{setting.Key}\n{setting.Label}")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't recall what the plan was- but if label is null- are we omitting the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we differentiate null vs. empty label? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
string featureFlagId = Convert.ToBase64String(featureFlagIdHash) | ||
.TrimEnd('=') | ||
.Replace('+', '-') | ||
.Replace('/', '_'); | ||
|
||
keyValues.Add(new KeyValuePair<string, string>($"{telemetryPath}:{FeatureManagementConstants.Metadata}:{FeatureManagementConstants.FeatureFlagId}", featureFlagId)); | ||
|
||
if (endpoint != null) | ||
{ | ||
string featureFlagReference = $"{endpoint.AbsoluteUri}kv/{setting.Key}{(setting.Label != null ? $"?label={setting.Label}" : "")}"; | ||
amerjusupovic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
keyValues.Add(new KeyValuePair<string, string>($"{telemetryPath}:{FeatureManagementConstants.Metadata}:{FeatureManagementConstants.FeatureFlagReference}", featureFlagReference)); | ||
} | ||
|
||
keyValues.Add(new KeyValuePair<string, string>($"{telemetryPath}:{FeatureManagementConstants.Metadata}:{FeatureManagementConstants.ETag}", setting.ETag.ToString())); | ||
|
||
keyValues.Add(new KeyValuePair<string, string>($"{telemetryPath}:{FeatureManagementConstants.Enabled}", telemetry.Enabled.ToString())); | ||
} | ||
} | ||
|
||
return Task.FromResult<IEnumerable<KeyValuePair<string, string>>>(keyValues); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move this inside the
if (telemetry.Enabled)
? No one cares about it if telemetry is not enabled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.