Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Automatically generate metrics docs using the Glean SDK #5101

Merged
merged 1 commit into from
Sep 12, 2019

Conversation

Dexterp37
Copy link
Contributor

@Dexterp37 Dexterp37 commented Sep 4, 2019

Create the docs from the Glean SDK metrics and ping files.

The current problem with this is that the file is generated in app/docs/metrics.md, but the existing file lives in docs/metrics.md.

Potential solutions:

Note: This depends on mozilla-mobile/android-components#4303

cc @sblatz @boek @liuche

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
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

@codecov-io
Copy link

codecov-io commented Sep 4, 2019

Codecov Report

Merging #5101 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5101      +/-   ##
============================================
+ Coverage     11.91%   11.93%   +0.02%     
  Complexity      245      245              
============================================
  Files           247      247              
  Lines         10231    10213      -18     
  Branches       1511     1507       -4     
============================================
  Hits           1219     1219              
+ Misses         8932     8914      -18     
  Partials         80       80
Impacted Files Coverage Δ Complexity Δ
.../java/org/mozilla/fenix/library/LibraryPageView.kt 0% <0%> (ø) 0% <0%> (ø) ⬇️
...a/org/mozilla/fenix/library/history/HistoryView.kt 0% <0%> (ø) 0% <0%> (ø) ⬇️
...lla/fenix/components/toolbar/BrowserToolbarView.kt 0% <0%> (ø) 0% <0%> (ø) ⬇️
...rg/mozilla/fenix/library/bookmarks/BookmarkView.kt 0% <0%> (ø) 0% <0%> (ø) ⬇️
.../bookmarks/viewholders/BookmarkFolderViewHolder.kt 0% <0%> (ø) 0% <0%> (ø) ⬇️
...ry/bookmarks/viewholders/BookmarkItemViewHolder.kt 0% <0%> (ø) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42b31f0...49ebb35. Read the comment docs.

@Dexterp37 Dexterp37 force-pushed the autodocs_glean branch 2 times, most recently from 17ca0fa to cda7905 Compare September 4, 2019 16:29
@Dexterp37 Dexterp37 marked this pull request as ready for review September 4, 2019 16:29
@Dexterp37 Dexterp37 requested review from a team as code owners September 4, 2019 16:29
@Dexterp37
Copy link
Contributor Author

This is now ready for review.

@Dexterp37
Copy link
Contributor Author

As a follow-up to this PR it might be useful for the Fenix team to change the current telemetry.md file to list the dependencies collecting metrics and link to their autogenerated docs.

@sblatz
Copy link
Contributor

sblatz commented Sep 10, 2019

@liuche who do we need to reach out to to ensure that this is sufficient Glean telemetry documentation?

@liuche
Copy link
Contributor

liuche commented Sep 11, 2019

Good question! I will follow up on this. I believe the Glean team has already checked, but I will make sure the final output fits the requirements.

This looks like quite a large diff, so I'm going to go through it and heavily spot-check it, and also get confirmation that this an acceptable format. Mostly looking for details on the extras, making sure those are all there.

Quick question @sblatz : Leanplum is documented in MMA.md - moving forward would we still need to handle the telemetry docs differently?
@Dexterp37 this doesn't handle Leanplum, right? and just ignores those.

@Dexterp37
Copy link
Contributor Author

Good question! I will follow up on this. I believe the Glean team has already checked, but I will make sure the final output fits the requirements.

There is an email thread (you should be CC'd on it @liuche ) in which we asked Michael Feldman about the requirements. We should be fine as long as the format is similar to what Fenix is currently using. For additional safety, we can ask Michael to review the generated file for Fenix. @liuche , are you taking care of that?

This looks like quite a large diff, so I'm going to go through it and heavily spot-check it, and also get confirmation that this an acceptable format. Mostly looking for details on the extras, making sure those are all there.

I can already tell you that static labels for labeled_ metric types are not there yet (bug). Once you merge this, changes will be automatically picked up next time somebody does a PR on this repo (after we fix the bug mentioned above). However, Fenix does not use labeled stuff with static labels (search stuff uses dynamic labels).

@Dexterp37 this doesn't handle Leanplum, right? and just ignores those.

No, this is Glean only. Any non-Glean documentation should be moved to a separate file. Alternatively, you could add some notice in the metrics description in metrics.yaml saying that "this is also sent via Leanplum". That way it will be automatically present in the generated docs. Given that there's only an handful of Leanplum metrics, I think it's about the same.

Note: this PR is aiming at a moving target. Every time a metric is added, this PR needs to be rebased in order to generate up-to-date docs :)

@liuche
Copy link
Contributor

liuche commented Sep 12, 2019

@Dexterp37 how does text added from metrics.yaml get represented in metrics.md - through the metrics description? Is description a top-level key in metrics.yaml that gets transcribed?

Copy link
Contributor

@liuche liuche left a comment

Choose a reason for hiding this comment

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

This looks good to me. It's just the automated version of metrics.yaml, and the metrics.md output is the same. This format also looks clear to me.

Next steps are to get this rebased/regenerated, and then we can land it.

@liuche
Copy link
Contributor

liuche commented Sep 12, 2019

@boek are there parts/steps of the Fenix documentation generation flow that can be removed now?

@Dexterp37
Copy link
Contributor Author

Dexterp37 commented Sep 12, 2019

@Dexterp37 how does text added from metrics.yaml get represented in metrics.md - through the metrics description? Is description a top-level key in metrics.yaml that gets transcribed?

The metric's description is one of the factors: we also extract metadata from the other fields, such as the data review, events keys and, in the near future, labels for labeled counters. The description itself is the biggest source of info.

We also analyse the pings.yaml, if available, in order to generate description for custom pings (see the activation ping.

Update our jinja2 template for markdown lives here

@liuche liuche added the pr:needs-landing PRs that are ready to land [Will be merged by Mergify] label Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants