Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Document labeled metrics types as separate individual types (bug 1553445) [ci skip] #3273

Merged
merged 1 commit into from Jun 7, 2019

Conversation

georgf
Copy link
Contributor

@georgf georgf commented Jun 5, 2019

This clearly calls out the labeled metric types as separate, individual types in the documentation.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

@georgf georgf requested a review from a team June 5, 2019 18:18
@georgf
Copy link
Contributor Author

georgf commented Jun 5, 2019

I'd like to get a first set of eyes on it, there are some clearly marked TODOs in it still.

  • If you can think of better examples, let me know.
  • For labeled_string & labeled_boolean i wonder if we have clear & good use-cases. If not, maybe we should disable this types for now and wait for use-cases to come up?

@georgf georgf changed the title Document labeled metrics types as separate individual types (bug 1553445) Document labeled metrics types as separate individual types (bug 1553445) [ci skip] Jun 5, 2019
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.

A few minor nits and a question about whether to include the restrictions on label names...

@Dexterp37 Dexterp37 requested a review from mdboom June 6, 2019 10:01
@Dexterp37
Copy link
Contributor

@georgf can you kindly file a bug for backporting these changes to glean-core?

@mdboom
Copy link
Contributor

mdboom commented Jun 6, 2019

@georgf can you kindly file a bug for backporting these changes to glean-core?

As I've brought docs over to glean-core, I've been very careful in the user docs to make sure it applies equally to glean-ac and glean-core. So at some point, maybe even before the code migration, we can probably replace all the docs in glean-ac with a link to the glean-core docs.

@georgf georgf marked this pull request as ready for review June 6, 2019 16:08
@georgf georgf requested a review from a team June 6, 2019 16:08
@georgf
Copy link
Contributor Author

georgf commented Jun 6, 2019

This is up for review now.

@georgf georgf requested review from travis79 and Dexterp37 June 6, 2019 16:08
@@ -80,15 +80,27 @@ Views.loginOpened...
There are different metrics to choose from, depending on what you want to achieve:

* [Events](#Events): Records events e.g. individual occurences of user actions, say every time a view was open and from where.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure where it should fit, but I now realize that we really want to express our opinion with respect to metric names: maybe we should explain why events.button_event isn't a good naming scheme (e.g. event is redundant as a category name and button_event doesn't really explain what the event is about). Any chance you can tackle this here on file a follow-up for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds orthogonal to the PR here.
Can you file a follow-up on what your intent there is @Dexterp37 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes them easier to explain and reason about them.
@georgf georgf merged commit cde8819 into mozilla-mobile:master Jun 7, 2019
mdboom added a commit to mdboom/glean.rs that referenced this pull request Jun 12, 2019
mdboom added a commit to mdboom/glean.rs that referenced this pull request Jun 12, 2019
mdboom added a commit to mozilla/glean that referenced this pull request Jun 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants