Track MSAL SKU for broker flows #7182
Conversation
konstantin-msft
commented
Jun 28, 2024
konstantin-msft
commented
- Track MSAL SKU for broker flows
- Enable server telemetry platform fields propagation
- Propagate broker error to server telemetry
konstantin-msft
requested review from
sameerag,
tnorling,
hectormmg,
jo-arroyo,
peterzenz and
Robbie-Microsoft
as code owners
github-actions
bot
added
msal-browser
konstantin-msft
force-pushed
the
report_msal_sku
branch
from
0216dc2
to
d84ae4a
Compare
lib/msal-browser/src/interaction_client/NativeInteractionClient.ts
Outdated
Show resolved
Hide resolved
lib/msal-browser/src/interaction_client/NativeInteractionClient.ts
Outdated
Show resolved
Hide resolved
| * @private | ||
| */ | ||
| private addRequestSKUs(request: NativeTokenRequest) { | ||
| request.extraParameters = { |
There was a problem hiding this comment.
Also, we should not modify input params. You should return a value instead
There was a problem hiding this comment.
This one modifies validatedRequest which is an extended copy of request. I do not think we need to add another spread operator here.
There was a problem hiding this comment.
We fixed a bug related to this pattern last week and I have a work item open to resolve remaining instances & turn on linting to catch in the future. We can discuss offline if you like.
lib/msal-browser/src/interaction_client/NativeInteractionClient.ts
Outdated
Show resolved
Hide resolved
- Enable server telemetry platform fields propagation - Propagate broker error to server telemetry
- Add unit tests
konstantin-msft
force-pushed
the
report_msal_sku
branch
from
818424a
to
8f7d6f2
Compare
|
|
||
| const BrokerServerParamKeys = { | ||
| BROKER_CLIENT_ID: "brk_client_id", | ||
| BROKER_REDIRECT_URI: "brk_redirect_uri", | ||
| }; | ||
|
|
||
| /** | ||
| * Provides MSAL and browser extension SKUs |
There was a problem hiding this comment.
Can you explicitly document what you expect to have in skus[0..4].
There was a problem hiding this comment.
Do we need to document it here in the code given that we have a shared spec?
| @@ -307,8 +365,13 @@ export class NativeInteractionClient extends BaseInteractionClient { | |||
| this.validateNativeResponse(response); | |||
| } catch (e) { | |||
| // Only throw fatal errors here to allow application to fallback to regular redirect. Otherwise proceed and the error will be thrown in handleRedirectPromise | |||
There was a problem hiding this comment.
Do you know if we document that if an error here happens, the application needs to retry? It feels like something we should handle for them.
There was a problem hiding this comment.
We fall back to the web flow if the native flow throws. cc @tnorling
