Updating extension telemetry #4996
Conversation
src/client/Microsoft.Identity.Client/Platforms/Features/OpenTelemetry/OtelInstrumentation.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/Internal/Requests/RequestBase.cs
Outdated
Show resolved
Hide resolved
Removing TokenType and replacing the values with string
| /// <summary> | ||
| /// Specifies the token type to log to telemetry. | ||
| /// </summary> | ||
| internal enum TokenType |
There was a problem hiding this comment.
Any reason for removing the enum? I liked the enum better, they are more readable and separates out the token types from all the other constants.
| /// <summary> | ||
| /// | ||
| /// </summary> | ||
| public string TelemetryTokenType { get; set; } |
There was a problem hiding this comment.
Can we add in the comment how this is different from the token type in authentication result?
| @@ -245,7 +245,7 @@ private ApiEvent InitializeApiEvent(string accountId) | |||
| AuthenticationRequestParameters.RequestContext.ServiceBundle.Config.LegacyCacheCompatibilityEnabled; | |||
|
|
|||
| apiEvent.CacheInfo = CacheRefreshReason.NotApplicable; | |||
| apiEvent.TokenType = (TokenType)AuthenticationRequestParameters.AuthenticationScheme.TelemetryTokenType; | |||
| apiEvent.TokenType = AuthenticationRequestParameters.AuthenticationScheme.TelemetryTokenType; | |||
There was a problem hiding this comment.
Do you think we might want to capture both TokenType and TelemetryTokenType in telemetry? I am a bit confused on what combinations are for these 2 values. Can you list some examples to help understand?
There was a problem hiding this comment.
My understanding is that the values are different only when the TelemetryTokenType is external. In that case, what will be the possible TokenTypes from ests?
|
|
||
| private void LogExtensionMetrics(AuthenticationResultMetadata authenticationResultMetadata, string platform, ApiEvent.ApiIds apiId) | ||
| { | ||
| if (authenticationResultMetadata.DurationCreatingExtendedTokenInUs <= 0) |
There was a problem hiding this comment.
Is it the case when this metrics is not captured? Can the condition be something like if TelemetryTokenType != Extension instead? Maybe 0 is a valid value in certain cases and should be captured?
| @@ -30,7 +31,8 @@ internal class OtelInstrumentation : IOtelInstrumentation | |||
| private const string DurationInL1CacheHistogramName = "MsalDurationInL1CacheInUs.1B"; | |||
| private const string DurationInL2CacheHistogramName = "MsalDurationInL2Cache.1A"; | |||
| private const string DurationInHttpHistogramName = "MsalDurationInHttp.1A"; | |||
| private const string DurationInExtensionInMsHistogram = "MsalDurationInExtensionInMs.1B"; | |||
|
|
|||
| private static Lazy<Dictionary<string, Histogram<long>>> ExtensionCounters = new(() => new Dictionary<string, Histogram<long> > ()); | |||
There was a problem hiding this comment.
| private static Lazy<Dictionary<string, Histogram<long>>> ExtensionCounters = new(() => new Dictionary<string, Histogram<long> > ()); | |
| private static Lazy<Dictionary<string, Histogram<long>>> ExtensionHistogram = new(() => new Dictionary<string, Histogram<long> > ()); |
Counters and histograms are 2 different concepts in otel. Naming a histogram as counter is confusing
| else | ||
| { | ||
| Histogram<long> newHistogram = Meter.CreateHistogram<long>( | ||
| $"MsalDuration{authenticationResultMetadata.TelemetryTokenType}Operation.1B", |
There was a problem hiding this comment.
How many histograms are we expecting to create? I am expecting only with the TelemetryTokenType as Extention?
There was a problem hiding this comment.
Did you consider capturing token type as a tag instead? I think that should give you similar results instead of creating different histograms considering you are capturing same data in all of them.
| return; | ||
| } | ||
|
|
||
| if (ExtensionCounters.Value.TryGetValue(authenticationResultMetadata.TelemetryTokenType, out Histogram<long> tokenCounter)) |
There was a problem hiding this comment.
Rename tokenCounter to extensionHistogram or something similar?
| LogExtensionMetrics(authResultMetadata, platform, apiId); | ||
| } | ||
|
|
||
| private void LogExtensionMetrics(AuthenticationResultMetadata authenticationResultMetadata, string platform, ApiEvent.ApiIds apiId) |
There was a problem hiding this comment.
Add comment on possible histograms being created here?
| { | ||
| get => TokenType.HasValue ? TokenType.Value.ToString("D") : null; | ||
| } | ||
| public string TokenType { get; set; } |
There was a problem hiding this comment.
This is a breaking change. The current value that is recorded in the server side telemetry is a numeric value of the enum. For example: 5|1004,2,,,|0,1,1. This change will break all the queries checking this value.
There was a problem hiding this comment.
Mainly, the token type is changed from enum to string which is a breaking change for server side telemetry. Also, would like to understand which histograms are created as part of this change. Can those be combined into a single histogram with tokenType as a tag?
Fixes #
Changes proposed in this request
Updating telemetry to always record the operation time used by the classes that implement
IAuthenticationOperation.Enabling the telemetry data to add histograms to telemetry based on the telemetry token type reported by
IAuthenticationOperation.Testing
Updated tests to cover new scenario
Performance impact
Negligible
Documentation