-
Notifications
You must be signed in to change notification settings - Fork 37
Add telemetry for the switch browser protocol #2612
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. |
) : IChallengeHandler<SwitchBrowserChallenge, Unit> { | ||
|
||
val span: Span by lazy { | ||
OTelUtility.createSpanFromParent(SpanName.SwitchBrowserProtocol.name, spanContext) |
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.
Is there not an existing span at this point? Seems excessive to create a new span for this
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.
Once we launch the intent to WebView, there is no active span in this context. You can look at other features that capture telemetry at this point and then create a new span.
Example: in AzureActiveDirectoryWebViewClient
is_sso_nonce_found_in_ests_request is not captured as the span doesn't exist.
We need to create spans like we do for ProcessNonceFromEstsRedirect, ProcessCrossCloudRedirect, Fido.
cc: @shahzaibj
Why parent span id is ATIInteractively? Why is it not AcquireTokenInteractive? |
I think prior to releasing this we need to increase the sampling rate for interactive requests, otherwise we will lose important data |
Since you are doing /process and /resume in separate span records, then why even have a generic That said, +1 to Fadi's comment, we should see if we can put the information directly on AcquireTokenInteractive spans to save cost. |
I think we can remove it, seems like an unnecessary span all this data can be in AcquireTokenInteractive span. But removing this can be dangerous and affect dashboards or materialized views, so we might want to do this in a different PR |
Yes, I am fine with that. |
Maybe we should flight it? |
Adding telemetry for Switch browser protocol.
How to read the data?
In conclusion, in a successful scenario, we should have 2 SwitchBrowserProtocol entries: one for the request and another for the resume, and the parent span_id should be the ATIInteractively span_id.
Example
android_spans
| where EventInfo_Time between(ago(1d)..now())
| where DeviceInfo_Model == "SM-G988U"
| where DeviceInfo_Id == "3f0fa7b42bed152f"
| project span_name,trace_id, span_id, parent_span_id, is_switch_browser_protocol, browser_package_name, is_custom_tabs_supported, is_switch_browser_request_handled, is_switch_browser_resume_handled
| order by trace_id
Related PR: https://github.com/AzureAD/ad-accounts-for-android/pull/3069