-
Notifications
You must be signed in to change notification settings - Fork 46
Using Baggage to propagate attributes from parent Span, Fixes AB#3262849 #2671
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
Conversation
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
...main/java/com/microsoft/identity/common/internal/providers/oauth2/AuthorizationActivity.java
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/opentelemetry/AttributeName.java
Show resolved
Hide resolved
...4j/src/main/com/microsoft/identity/common/java/opentelemetry/TextMapPropagatorExtension.java
Outdated
Show resolved
Hide resolved
| */ | ||
| public final class TextMapPropagatorExtension { | ||
|
|
||
| private static final TextMapPropagator PROPAGATOR = TextMapPropagator.composite( |
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.
Can you check this (and any new otel code introduced in this PR i.e. methods we were previously not using) if they need to wrapped in a try/catch block to make it safe for older devices. This is what the other "extension" classes written for otel are doing to protect against exceptions that fall into the MethodNotAvailable sort of category on older devices.
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.
Noted; I added try/catch blocks in the methods of the new extension classes I added, as well as new methods I added to existing extension classes.
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.
Pull Request Overview
This PR implements OpenTelemetry Baggage propagation to attach attributes from a parent span directly onto child spans, reducing reliance on separate joins by trace ID.
- Introduces helper extensions for context and baggage injection/extraction.
- Updates activities and WebView client to carry OTEL context through intents and runtime.
- Enhances FIDO challenge handler to read baggage and set span attributes directly.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| TextMapPropagatorExtension.java | Add utilities to inject/extract OTEL context with trace and baggage propagators |
| SpanExtension.java | Add safe fromContext wrapper and expose public NoopScope |
| OtelContextExtension.java | Add safe current() method to retrieve OTEL context |
| NoopBaggage.java | Implement no-op Baggage with placeholder methods |
| BaggageExtension.java | Provide methods to make baggage current and extract from context |
| AttributeName.java | Add new span attribute keys (tenant_id, account_type, etc.) |
| AuthFidoChallengeHandlerTest.kt | Rename test parameters to use OTEL context |
| AzureActiveDirectoryWebViewClient.java | Switch to use OTEL Context instead of SpanContext |
| AuthorizationActivityFactory.kt | Inject OTEL context carrier into intent extras |
| AuthorizationActivity.java | Extract OTEL context from intent and store on activity |
| AuthFidoChallengeHandler.kt | Read baggage entries and set span attributes for FIDO spans |
| AuthenticationConstants.java | Define OTEL_CONTEXT_CARRIER intent key |
| changelog.txt | Add entry for Baggage propagation feature |
Comments suppressed due to low confidence (2)
common4j/src/main/com/microsoft/identity/common/java/opentelemetry/OtelContextExtension.java:87
- The Javadoc for
current()states it returns a root context on error, but the method actually returnsnull. Update the doc or returnContext.root()to align behavior with the description.
return null;
common4j/src/main/com/microsoft/identity/common/java/opentelemetry/TextMapPropagatorExtension.java:60
- There are no unit tests validating the
inject/extractbehavior. Adding tests forTextMapPropagatorExtension(andBaggageExtension) will ensure context propagation is correctly serialized and deserialized.
public static HashMap<String, String> inject(final Context context) {
common4j/src/main/com/microsoft/identity/common/java/opentelemetry/NoopBaggage.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/microsoft/identity/common/internal/fido/AuthFidoChallengeHandler.kt
Outdated
Show resolved
Hide resolved
common4j/src/main/com/microsoft/identity/common/java/opentelemetry/AttributeName.java
Show resolved
Hide resolved
fadidurah
left a comment
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.
Approved from code POV, but how are we validating these changes?
...main/java/com/microsoft/identity/common/internal/providers/oauth2/AuthorizationActivity.java
Outdated
Show resolved
Hide resolved
...4j/src/main/com/microsoft/identity/common/java/opentelemetry/TextMapPropagatorExtension.java
Outdated
Show resolved
Hide resolved
I can validate the change by checking the broker sandbox telemetry (where I see that my test login entries have data for the columns we were previously associating with a join). Also, Shahzaib mentioned adding a flight in the broker PR, so I've done that too and am validating that that works with ECS INT. |
Using baggage to get parent span's (AcquireTokenInteractive) correlationId, calling app package name and setting them on the child spans created during webcp redirects in webview. Why this needed? - We do not have correlationId set on webcp spans as of today. Although, we can check for the parent span and get correlationId from there, it is just an additional step. I want to avoid this. - We cannot directly set attributes to indicate that is webcp flow on the parent span (ATI) since the context passed to the activity is immutable. Recommended way is to create a child span and set attributes as needed on it. - We also cannot get parent span's attributes on child span directly using the context passed. Support for this was added by using baggage in this PR #2671. So, I am leveraging this. Related broker PR to remove the redundant attributes : AzureAD/ad-accounts-for-android#3230 Fixes [AB#3385536](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3385536)
Summary
The passkey telemetry dashboard (and maybe some others) has had issues with latency due to joins being done between the Fido span and the parent span related to acquire token interactive. These join statements by trace id allowed us to associate attributes (such as tenant id) to the Fido span that weren't available in the context of the Fido classes.
In order to directly save the attributes onto the Fido span itself, we can make use of Otel's Baggage, which is meant to help hold data across multiple spans (from parent to children). We can then use an implementation of Otel's TextMapPropagator to serialize the Otel Context and pass it to the AuthenticationActivity as an Extra (similar to what we are doing for spanContext). This Context can then be accessed by the WebViewClient and Fido related classes, where they will retrieve the Baggage.
This PR has the changes to implement this behavior and adds a few static helper classes for TextMapPropagator and Baggage (the baggage class is more relevant for the corresponding Broker PR: https://github.com/AzureAD/ad-accounts-for-android/pull/3119).
While this PR tackles attribute propagation for AuthorizationActivity specifically, Baggage and the helper classes can be used in other areas of the code where propagation is needed as well.
AB#3262849