Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

Add telemetry for uri count. #540

Merged
merged 1 commit into from
Sep 24, 2018
Merged

Add telemetry for uri count. #540

merged 1 commit into from
Sep 24, 2018

Conversation

daoshengmu
Copy link
Contributor

Update mozilla.components:telemetry to 0.23 for using large size of histograms.

Follow the requirement for the product side to understand URI count as Focus team (mozilla-mobile/focus-android#3320 (comment)) and add the histogram to know the avg. time spent for the page loading.

@pocmo Please help review the telemetry part. I believe we need a data review as well.

bluemarvin
bluemarvin suggested changes Sep 18, 2018
View reviewed changes
Copy link
Contributor

@bluemarvin bluemarvin left a comment

Choose a reason for hiding this comment

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

It isn't clear that this is doing what you expect. Have you tested that the domains are getting created correctly and we are not uploading user data with the domains?

app/src/common/shared/org/mozilla/vrbrowser/utils/UrlUtils.java Outdated Show resolved Hide resolved
app/src/common/shared/org/mozilla/vrbrowser/utils/UrlUtils.java Outdated
return host.substring(start);
}

public static boolean isLocalizedContent(@Nullable String url) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this function actually does anything right now. If it is supposed to filter out local content like error pages and the home page, it needs to do a lot more than just watch for about:blank

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making isLocalizedContent () to check if it is PRIVATE_BROWSING_URI, HOME_WITHOUT_REGION_ORIGIN, getHomeUri, and about:blank

app/src/common/shared/org/mozilla/vrbrowser/telemetry/TelemetryWrapper.java Outdated Show resolved Hide resolved
@daoshengmu
Copy link
Contributor Author

yes. I have tested. For example http://aframe.io/examples/showcase/videosphere/, it only puts aframe.io to the hash table. Besides, we will not upload user data. //We only upload the domain and URI counts to the probes without including users' URI info.

@daoshengmu daoshengmu force-pushed the uriCountTelemetry branch 2 times, most recently from d88ea18 to 3e4088f Compare September 19, 2018 16:40
@daoshengmu daoshengmu self-assigned this Sep 19, 2018
@bluemarvin
Copy link
Contributor

yes. I have tested. For example http://aframe.io/examples/showcase/videosphere/, it only puts aframe.io to the hash table. Besides, we will not upload user data. //We only upload the domain and URI counts to the probes without including users' URI info.

Sorry, I miss read the code.

@bluemarvin bluemarvin merged commit bceab33 into MozillaReality:master Sep 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cvan cvan cvan left review comments

@bluemarvin bluemarvin bluemarvin approved these changes

Assignees

@daoshengmu daoshengmu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daoshengmu @bluemarvin @cvan