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

Fix #6886 - Telemetry for New Tab Button on Url Nav bar #7023

Merged
merged 2 commits into from
Jul 23, 2020
Merged

Fix #6886 - Telemetry for New Tab Button on Url Nav bar #7023

merged 2 commits into from
Jul 23, 2020

Conversation

nbhasin2
Copy link
Contributor

No description provided.

nbhasin2
nbhasin2 commented Jul 23, 2020
View reviewed changes
Client/metrics.yaml Outdated Show resolved Hide resolved
@nbhasin2 nbhasin2 linked an issue Jul 23, 2020 that may be closed by this pull request
FXIOS-563 ⁃ Telemetry for New Tab Button on Nav #6886
Closed
@nbhasin2
Copy link
Contributor Author

@travis79 I have added an event after and updated the metrics.yaml

Could you please check the two comments I have added above. They are mainly to do with following question:

  • How to generate the metrics.swift file?
  • Is the metrics.yaml file added properly?

@mozillamobilebot
Copy link

SwiftLint found issues

Warnings

File Line Reason
TelemetryWrapper.swift 380 Prefer empty collection over optional collection. (discouraged_optional_collection)
TelemetryWrapper.swift 386 Prefer empty collection over optional collection. (discouraged_optional_collection)
TelemetryWrapper.swift 392 Prefer empty collection over optional collection. (discouraged_optional_collection)

Errors

File Line Reason
TelemetryWrapper.swift 383 Lines should not have trailing semicolons. (trailing_semicolon)
TelemetryWrapper.swift 389 Lines should not have trailing semicolons. (trailing_semicolon)

Generated by 🚫 Danger

travis79
travis79 suggested changes Jul 23, 2020
View reviewed changes
Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

I think there is a Glean Parser lint or error that is preventing it from generating the SDK code, please check the build errors and if you don't see anything there, ping me and I'll take a look at why it's not working for you.

Docs/telemetry.md
@@ -227,6 +227,12 @@ The event ping contains a list of events ([see event format on readthedocs.io](h
|---------------------------------------------------------------------|----------|-----------|---------|------------------|---------------------------------------------------|
| Allow user to search or add url from tab tray search button | action | tap | start-search-button | -n/a- | -n/a- |

### Url Bar

| Event | category | method | object | value | extras |
Copy link
Member

Choose a reason for hiding this comment

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

If the code generation for the Glean SDK went correctly, it would have updated the /docs/metrics.md file and you should no longer have to manually document your telemetry like this. This should only be necessary for legacy telemetry.

Client/Telemetry/TelemetryWrapper.swift Outdated Show resolved Hide resolved
Client/metrics.yaml Outdated
@@ -325,6 +325,18 @@ tabs:
notification_emails:
- firefox-ios@mozilla.com
expires: "2021-07-01"
add-new-tab-button:
Copy link
Member

Choose a reason for hiding this comment

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

I don't see the update to the metrics.md or anything that suggests that this was processed by the glean_parser build step correctly. If you look at the errors to the build, do you see any "Glean Parser" errors or warnings? If so, you need to fix those for the code generation step to work.

Also, just a naming nit, but this means you will be calling this like GleanMetrics.Tabs.addNewTabButton.add(), which is a little awkward. What if you shortened it to new-tab-button?

Copy link
Member

Choose a reason for hiding this comment

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

Is this different than the tab open counter above? I understood it to be the event that fired when a new tab is opened, but this is fine if there are multiple ways of opening a new tab and you this is just counting one of the ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we have a new tab button at the tab toolbar and this one is on the url nav bar hence the difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@travis79 Took your suggestion and renamed it to be new_tab_pressed :)

@nbhasin2 nbhasin2 requested a review from travis79 July 23, 2020 15:24
travis79
travis79 approved these changes Jul 23, 2020
View reviewed changes
Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

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

This LGTM! Please let me know if you run into any more issues :)

@nbhasin2 nbhasin2 requested a review from dnarcese July 23, 2020 15:26
@nbhasin2 nbhasin2 merged commit 1fee70a into mozilla-mobile:main Jul 23, 2020
@nbhasin2 nbhasin2 deleted the nb/6886-Telemetry_New_TabButton branch July 23, 2020 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnarcese dnarcese dnarcese approved these changes

@travis79 travis79 travis79 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FXIOS-563 ⁃ Telemetry for New Tab Button on Nav
4 participants
@nbhasin2 @mozillamobilebot @travis79 @dnarcese