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.

Sign up for GitHub

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

Jump to bottom

Integrate the telemetry events #565

Merged
merged 20 commits into from
Sep 5, 2019
Merged

Integrate the telemetry events #565

merged 20 commits into from
Sep 5, 2019

Conversation

heidijinxujia
Copy link
Member

@heidijinxujia heidijinxujia commented Jul 27, 2019 •
edited
Loading

Update the api events and add constants for telemetry string.

Related PR for MSAL: AzureAD/microsoft-authentication-library-for-android#692

@heidijinxujia heidijinxujia self-assigned this Jul 27, 2019
@heidijinxujia
Integrate UI events
70f5141
@heidijinxujia
Merge branch 'dev' into jinjia/apiEvents
7d1577b 
* dev: (34 commits)
  Use the junit version declared in versions.gradle (#588)
  Adding junit version declaration to versions.gradle (#587)
  Update Travis-CI build SDK version 27 -> 28, Fix AndroidX Compat (#581)
  Throttle Telemetry with Shared pref on broker switch (#583)
  Version + change log update
  Set the InteractiveTokenCommand to null once it has finished (#580)
  Updated log
  Formatting changes
  Adding null safety check to avoid crash if broadcast is sent multiple times
  Migrate to AndroidX (#575)
  Removing commented out line (#578)
  Change log update
  Version update to remove RC
  Read user info id from the request bundle on silent request (#574)
  New CI config (#573)
  Adding flexibility around pwd_exp return type, string, int, long (#572)
  Disabling tests on travis
  Change log update
  Bump common version to 0.0.12
  Fix True MAM silent call scenerio (#570)
  ...
@heidijinxujia
Add the events.
745777a
@heidijinxujia
Merge branch 'dev' into jinjia/apiEvents
5540e4c 
* dev:
  First pass, app registry implementation (#591)
@heidijinxujia
Update events and constants
c5c5f37
@heidijinxujia
Update the constants to align with ADAL
1dee216 
Fix the correlation id bug
@heidijinxujia
Fix the counting and updated fields to align with ADAL telemetry
e150971
@heidijinxujia
Merge branch 'dev' into jinjia/apiEvents
9cb168d 
* dev:
  Handle the app installation link for browser flow (#611)
  Correcting javadoc (#610)
  Fix the back-compat of hello() (#606)
  Closes #607, #604 (#608)
  Adds implementation for OpenIdConfig (#605)
  Add PCA initialization error strings (#598)
  Change log update
  Do not load access tokens from foci cache fallback logic (#599)
  Update to 0.0.16 (#596)
  Add clearBrokerSecretKeys() (#595)
  Fix for foci lookups relative to migration (#594)
  Change telemetry type from 'session' to 'event' (#592)
@heidijinxujia
To fix the broker NPE caused by common telemetry.
4955d8e
@heidijinxujia heidijinxujia changed the title [WIP] Integrate the telemetry events Integrate the telemetry events Sep 3, 2019
@heidijinxujia heidijinxujia mentioned this pull request Sep 3, 2019
Merged
6 tasks
iambmelt
iambmelt requested changes Sep 3, 2019
View reviewed changes
Copy link
Member

@iambmelt iambmelt left a comment

Choose a reason for hiding this comment

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

Unit tests are failing

@heidijinxujia
Add the MRRT status as TRUE by default.
b529270 
Fix the failed unit tests.
@heidijinxujia
Merge branch 'dev' into jinjia/apiEvents
56b32f3 
* dev:
  Added some additional logging
@heidijinxujia
Copy link
Member Author

@iambmelt Thank you, Brian. Fixed.

@iambmelt
Copy link
Member

iambmelt commented Sep 3, 2019

@heidijinxujia Correct me if I'm wrong; looks like there's a handful of scenarios not fully captured here.

Caching

  • SSO state sync
  • ADAL to MSAL upgrade
  • FoCI lookups
  • save() actions

Am I correct in assuming these are TODO?

@heidijinxujia
Copy link
Member Author

@iambmelt Added the issue here. #627

@iambmelt
Copy link
Member

iambmelt commented Sep 4, 2019

@iambmelt Added the issue here. #627

Great, thanks for capturing this

iambmelt
iambmelt requested changes Sep 4, 2019
View reviewed changes
Copy link
Member

@iambmelt iambmelt left a comment

Choose a reason for hiding this comment

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

Couple of questions, suggestions around calculateEventResponseTime

rpdome
rpdome reviewed Sep 4, 2019
View reviewed changes
Copy link
Member

@rpdome rpdome left a comment

Choose a reason for hiding this comment

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

left a few comments. Thanks!

.../src/main/java/com/microsoft/identity/common/adal/internal/net/DefaultConnectionService.java
@@ -74,15 +79,18 @@ public boolean isNetworkDisabledFromOptimizations() {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {
final UsageStatsManagerWrapper usageStatsManagerWrapper = UsageStatsManagerWrapper.getInstance();
if (usageStatsManagerWrapper.isAppInactive(mConnectionContext)) {
Telemetry.emit((BaseEvent) new BaseEvent().put(TelemetryEventStrings.Key.POWER_OPTIMIZATION, String.valueOf(true)));
Copy link
Member

Choose a reason for hiding this comment

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

Would this be misleading? (as 'true' here means Broker app is in standby - it doesn't necessary means that power optimization is on).

Copy link
Member Author

Choose a reason for hiding this comment

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

The aim of this function isNetworkDisabledFromOptimizations() is to check if the network is not functional because of power optimization.

Copy link
Member

Choose a reason for hiding this comment

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

Is that the same for telemetry event?

Copy link
Member Author

Choose a reason for hiding this comment

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

The telemetry here is to track the power optimization is on. The telemetry for the network unavailable is in the calling method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please let me know if you prefer another naming of the property.

...main/java/com/microsoft/identity/common/internal/providers/oauth2/AuthorizationActivity.java Show resolved Hide resolved
...on/src/main/java/com/microsoft/identity/common/internal/telemetry/TelemetryEventStrings.java Show resolved Hide resolved
...va/com/microsoft/identity/common/internal/telemetry/adapter/TelemetryAggregationAdapter.java Outdated Show resolved Hide resolved
@heidijinxujia
Address the comments.
17f6f77
@heidijinxujia
Merge branch 'dev' into jinjia/apiEvents
e34b893 
* dev:
  Add keyvault and labapi to common's settings.gradle
  Add keyvault and labapi to common
@heidijinxujia
Address the comments.
1b5eb34
@heidijinxujia
Copy link
Member Author

@iambmelt Thanks a lot for the reviews, could you have another round of look?

@heidijinxujia heidijinxujia merged commit 19bf142 into dev Sep 5, 2019
@iambmelt iambmelt deleted the jinjia/apiEvents branch February 6, 2020 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpdome rpdome rpdome left review comments

@shahzaibj shahzaibj shahzaibj left review comments

@iambmelt iambmelt iambmelt approved these changes

@kreedula kreedula Awaiting requested review from kreedula

@shoatman shoatman Awaiting requested review from shoatman

Assignees

@heidijinxujia heidijinxujia

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@heidijinxujia @iambmelt @rpdome @shahzaibj