Skip to content
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

Add wrapper functions for telemetry & logging #6383

Merged
merged 14 commits into from
Aug 29, 2023
Merged

Conversation

tnorling
Copy link
Collaborator

@tnorling tnorling commented Aug 23, 2023

  • Introduces invoke and invokeAsync helper functions to wrap APIs with telemetry measurements and trace logging.
  • Adds perf measurements for ssoSilent/iframed flows
  • Moves httpVerToken and refreshTokenSize to more central place right after POST
  • Remove httpVerAuthority is it represents the same thing as httpVerToken but with a different name

Pros

  • Makes it easier and less verbose to add logging & take perf measurements
  • In theory should be less expensive from a package size perspective

Cons

  • Makes code a bit harder to read
  • If a function is called from multiple places each callsite must wrap the function (in contrast to being baked in today)

@github-actions github-actions bot added the msal-browser Related to msal-browser package label Aug 23, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2023

Codecov Report

Merging #6383 (ac09dde) into dev (81d34b4) will decrease coverage by 1.94%.
Report is 278 commits behind head on dev.
The diff coverage is 60.00%.

Flag Coverage Δ
msal-angular 96.73% <ø> (+0.22%) ⬆️
msal-browser 85.70% <ø> (-0.77%) ⬇️
msal-common 82.85% <ø> (-1.70%) ⬇️
msal-core ?
msal-node 80.01% <ø> (-3.38%) ⬇️
msal-node-extensions 67.37% <59.74%> (-8.27%) ⬇️
msal-react 94.24% <ø> (-0.45%) ⬇️
node-token-validation ?
Files Changed Coverage Δ
...ions/msal-node-extensions/src/utils/Environment.ts 20.51% <0.00%> (-22.08%) ⬇️
lib/msal-angular/src/constants.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.broadcast.service.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.guard.ts 90.78% <ø> (+0.64%) ⬆️
lib/msal-angular/src/msal.interceptor.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.module.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.navigation.client.ts 93.33% <ø> (+0.47%) ⬆️
lib/msal-angular/src/msal.redirect.component.ts 100.00% <ø> (ø)
lib/msal-angular/src/msal.service.ts 100.00% <ø> (ø)
lib/msal-angular/src/packageMetadata.ts 100.00% <ø> (ø)
... and 65 more

... and 167 files with indirect coverage changes

@github-actions github-actions bot added msal-node Related to msal-node package msal-common Related to msal-common package labels Aug 24, 2023
Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us track the user experience with a separate PR, approving as is. This is great improvement over redundant code.

Copy link
Contributor

@peterzenz peterzenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a nice readability and maintenance improvement. Thanks.

@@ -74,11 +75,6 @@ export class SilentIframeClient extends StandardInteractionClient {
PerformanceEvents.SilentIframeClientAcquireToken,
request.correlationId
);
this.logger.verbose("acquireTokenByIframe called");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend to remove this logger line as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the invoke functions will handle logging start/end messages as well

@tnorling tnorling enabled auto-merge (squash) August 29, 2023 18:28
@tnorling tnorling merged commit ca09bbd into dev Aug 29, 2023
42 checks passed
@tnorling tnorling deleted the wrap-with-telem-logging branch August 29, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package msal-common Related to msal-common package msal-node Related to msal-node package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants