-
Notifications
You must be signed in to change notification settings - Fork 7
fix(metrics.yaml): include new data_taxonomy & data_reviews info #86
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kirill-demtchouk I copied over your metrics.yaml
file and then made the following changes in order for it to work on web:
-
removed
fxa_account_id
,adjust_device_id
andbackend
category as these are not relevant to web -
added
bugs: TBD
back to each metric as thebuild:glean
script fails without it -
moved
baseline
out ofsend_in_pings
and intono_lint
- the
build:glean
script fails if we keep as is - alternatively, web could remove
baseline
altogether
- the
-
added back
lifetime: application
to the non-event metrics- this is what makes it so that those identifiers get sent with every ping and I have yet to figure out a different way to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can we keep the
fxa_account_id
, even if we're not using it at the moment? - What does
no_lint
do? I can't find any mention of it in the Glean docs 🙃
I think the other changes are 👌 , thank you for making them!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, can we set the bugs
field to a value of https://mozilla-hub.atlassian.net/browse/MSDO-49
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback @kirill-demtchouk! I made some changes:
- added
https://mozilla-hub.atlassian.net/browse/MSDO-49
as the bugs field to all metrics - added back the
fxa_account_id
- fixed a couple tiny typos (I'll call them out so you can make the changes in your metrics.yaml file as well)
The no_lint
line suppresses the warning we get when running the build
or lint
commands. If we leave baseline
as-is, we get this error:
ERROR: BASELINE_PING: identifiers.fxa_account_id: The baseline ping is Glean-internal. Remove 'baseline' from the send_in_pings array.
If you'd rather we just remove the baseline
instead of classifying it as no_lint
, that would be fine with me, just let me know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wtfluckey Let's remove the baseline
ping since it's not relevant to the Glean js SDK. I will make those typo changes on my side. Thank you for catching them!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm feeling sick and will likely be offline this afternoon (and I'm out next week on PTO). Going to approve this PR to unblock you, it's not material for me to review the baseline
ping change alone.
This looks good n' compliant to me 😄 Thank you for hashing out the last edits!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all your help @kirill-demtchouk!! Also, I hope you feel better and have an excellent thanksgiving!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Aly! One last thing: can we changed the "app_id" defined in the code to "moso-mastodon-web"? It's currently set to "mozilla-social-web".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done as part of #89
telemetry/generated/web.ts
Outdated
import EventMetricType from "@mozilla/glean/private/metrics/event"; | ||
import StringMetricType from "@mozilla/glean/private/metrics/string"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woohoo I've been ignoring this particular generated change in my local
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about metrics.yaml with the open comment, but I understand that the changes to that file in this PR are part of one entire file copy. Everything else looks good to me.
63a39e8
to
eed66aa
Compare
type: string | ||
ui_additional_detail: &ui_additional_detail | ||
description: > | ||
An optional string to record further informatin about the UI | ||
An optional string to record further information about the UI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kirill-demtchouk - fixed a typo here
@@ -170,13 +276,19 @@ web: | |||
description: > | |||
The full URL of the previous web page from which a link was followed | |||
in order to trigger the page view. | |||
Comes from the `referer` field of the HTTP header. | |||
Comes from the `referrer` field of the HTTP header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kirill-demtchouk - one other typo here
eed66aa
to
541cbf6
Compare
@wtfluckey should we start sending events once this PR is merged? |
Update our
metrics.yaml
file to match the shared metrics.yaml file to include the newdata_taxonomy
anddata_reviews
informationRequesting review from Kirill as well since I made changes to the
metrics.yaml
file and we are trying to keep it as consistent as possible across clients and want to make sure that the changes I've made here don't violate our legal requirements.Completed:
bugs
info to all metricsdata_reviews
info to all metricsdata_taxonomy
for all metricsfxa_account_id
metric per Kirill's request