Bug 1723090 - Delete trailing glean.restarted events #1452

rosahbruno · 2022-08-24T17:23:08Z

Description

Add functionality to delete all trailing glean.restarted events. Now before returning the formatted events for the ping, we pop items from the array until we hit one that isn't a restarted event. If the last item is not a glean.restarted event, then it will not do anything.

Glean Book Documentation PR - mozilla/glean#2172

Pull Request checklist

Quality: Make sure this PR builds and runs cleanly.
- Inside the glean/ folder, run:
  - npm run test Runs all tests
  - npm run lint Runs all linters
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry to CHANGELOG.md or an explanation of why it does not need one
Documentation: This PR includes documentation changes, an explanation of why it does not need that or a follow-up bug has been filed to do that work
- Documentation should be added to The Glean Book when making changes to Glean's user facing API
  - When changes to the Glean Book are necessary, link to the corresponding PR on the mozilla/glean repository
- Documentation should be added to the Glean.js developer documentation when making internal code changes

When we send a ping, we want to avoid passing unnecessary trailing restarted events. Before we return the payload, we can step backwards through our events array and pop each restarted event until we hit an event that is a non-restarted event. At this point we return the array since all trailing events have been removed. In the case that the array does not have a trailing restarted event, we avoid invoking the new functionality completely.

We generate an array of events, in a somewhat unorthodox manner to ensure that our original array contains multiple trailing restarted events. We then put a few restarted events between non-restarted events to verify that we stop removing items once we hit a non-restarted event.

We no longer keep `glean.restarted` events at the end of the events array, so the tests needed to be updated to reflect that. The tests check everything the same way, they just no longer check that the final item is a `glean.restarted` event.

rosahbruno · 2022-08-24T18:56:57Z

I want to get a second opinion on what should be updated in the docs. If you look under glean.restarted there is verbiage that I am not entirely sure is accurate anymore. https://github.com/mozilla/glean.js/blob/9186c5aa38ab6a35b184137e6c8f8985b771e5d8/docs/reference/metrics.md#all-pings

We now strip the first restarted event from the events array as well as all trailing events. I may lack enough context to know for sure, but I would think that there is a possibility now where that event doesn't get passed at all.

I am going to do some testing with the sample projects to try and confirm as well.

glean/tests/unit/core/metrics/events_database.spec.ts

glean/tests/unit/core/pings/maker.spec.ts

glean/src/core/metrics/events_database/index.ts

Found a more straightforward way to trigger a restart without forcing an error.

- Additional clarifying comments - More descriptive variable names for mock events array values.

badboy · 2022-08-26T10:09:21Z

Just noticed that I wrote some comments, but didn't submit them -_-

badboy

Code looks good, but PR is still in draft mode. If you're ready turn it into a full PR please.

badboy · 2022-08-26T10:16:36Z

I want to get a second opinion on what should be updated in the docs. If you look under glean.restarted there is verbiage that I am not entirely sure is accurate anymore. 9186c5a/docs/reference/metrics.md#all-pings

We now strip the first restarted event from the events array as well as all trailing events. I may lack enough context to know for sure, but I would think that there is a possibility now where that event doesn't get passed at all.

I am going to do some testing with the sample projects to try and confirm as well.

I think that wording is still correct, though is leaving out a bit of context.
In general glean.restarted can end up in any ping with events, without the user needing to define anything extra.
Whether a specific ping with events has that particular event depends on whether the application was actually restarted.
Even if we strip the first and all trailing restarted events it might still occur in the middle.

rosahbruno · 2022-08-26T11:24:03Z

Code looks good, but PR is still in draft mode. If you're ready turn it into a full PR please.

Was just waiting on some feedback for the docs part since I knew that was going to still need updating before publishing for real. I will update the docs with that additional context, look at triggering restarts without an error, then publish this.

@badboy thanks for the multiple reviews!

badboy · 2022-08-26T11:36:16Z

Code looks good, but PR is still in draft mode. If you're ready turn it into a full PR please.

Was just waiting on some feedback for the docs part since I knew that was going to still need updating before publishing for real. I will update the docs with that additional context, look at triggering restarts without an error, then publish this.

yeah, I'm not sure how to concisely add that for the single metric.
We explain glean.restarted a tiny bit more in the Glean SDKs book.

rosahbruno · 2022-08-26T12:45:04Z

Code looks good, but PR is still in draft mode. If you're ready turn it into a full PR please.

Was just waiting on some feedback for the docs part since I knew that was going to still need updating before publishing for real. I will update the docs with that additional context, look at triggering restarts without an error, then publish this.

yeah, I'm not sure how to concisely add that for the single metric. We explain glean.restarted a tiny bit more in the Glean SDKs book.

@badboy
I added a small clarification in the metrics.md docs just to note that those leading and trailing restarted events will be omitted. I can add a note in the Glean SDKs book too, since there is already that JS specific section.

a8f8ed4

- generate barrel file to better manage `testing/core` exports - create testing util that restarts glean by creating a new instance of the `EventsDatabase` - update existing and new tests to use new restart helper

rosahbruno · 2022-08-26T15:07:00Z

glean/src/core/testing/index.ts

  await testInitializeGlean(applicationId, uploadEnabled, config);
 }
+
+export * from "./utils.js";


Clarification for this

A more common pattern in JS/TS is to bubble all of your folder/module exports up to an index.ts file, "barrel file", and export everything from there.

That way when you are accessing different functions/variables/constants you only have to drill down to the top level of the folder when importing. So instead of pulling 3 things from index, 4 things from utils, etc, you re-export everything you need from the index then you can just import it all via import {} from '../core/testing.

This also helps to keep things more organized and modular. You start to see very quickly what may be tightly coupled to a module that doesn't need to be.

rosahbruno · 2022-08-26T15:30:30Z

PR is now ready for review. I've made all PR changes as well as create a documentation update PR for the Glean Book.

rosahbruno added 3 commits August 24, 2022 13:26

Update tests for trailing events changes

77e8cdd

We no longer keep `glean.restarted` events at the end of the events array, so the tests needed to be updated to reflect that. The tests check everything the same way, they just no longer check that the final item is a `glean.restarted` event.

rosahbruno force-pushed the 1723090-delete-trailing-restart-events branch from 6d4ea73 to 77e8cdd Compare August 24, 2022 17:27

Update changelog for mozilla#1452

15fb3fc

badboy reviewed Aug 25, 2022

View reviewed changes

rosahbruno added 3 commits August 25, 2022 10:56

Simplify call for triggering a restart

432dd90

Found a more straightforward way to trigger a restart without forcing an error.

Add clarifying comment to isRestartedEvent

7c06436

Readability improvements for events tests

9772c2e

- Additional clarifying comments - More descriptive variable names for mock events array values.

rosahbruno force-pushed the 1723090-delete-trailing-restart-events branch from 54fbc1e to 9772c2e Compare August 25, 2022 18:18

badboy reviewed Aug 26, 2022

View reviewed changes

Clarification in metrics docs for glean.restarted

a8f8ed4

Create more generic helper for faking glean restarts

b5fe0a1

- generate barrel file to better manage `testing/core` exports - create testing util that restarts glean by creating a new instance of the `EventsDatabase` - update existing and new tests to use new restart helper

rosahbruno force-pushed the 1723090-delete-trailing-restart-events branch from 1bb3d33 to b5fe0a1 Compare August 26, 2022 15:06

rosahbruno commented Aug 26, 2022

View reviewed changes

rosahbruno mentioned this pull request Aug 26, 2022

Bug 1723090 - Add note to glean.restarted docs about leading/trailing event omission mozilla/glean#2172

Merged

rosahbruno marked this pull request as ready for review August 26, 2022 15:30

auto-assign bot requested a review from Dexterp37 August 26, 2022 15:30

badboy approved these changes Aug 26, 2022

View reviewed changes

rosahbruno merged commit f2b2880 into mozilla:main Aug 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bug 1723090 - Delete trailing glean.restarted events #1452

Bug 1723090 - Delete trailing glean.restarted events #1452

Uh oh!

rosahbruno commented Aug 24, 2022 •

edited

Loading

Uh oh!

rosahbruno commented Aug 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

badboy commented Aug 26, 2022

Uh oh!

badboy left a comment

Uh oh!

badboy commented Aug 26, 2022

Uh oh!

rosahbruno commented Aug 26, 2022

Uh oh!

badboy commented Aug 26, 2022

Uh oh!

rosahbruno commented Aug 26, 2022

Uh oh!

rosahbruno Aug 26, 2022

Uh oh!

rosahbruno commented Aug 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Bug 1723090 - Delete trailing glean.restarted events #1452

Bug 1723090 - Delete trailing glean.restarted events #1452

Uh oh!

Conversation

rosahbruno commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull Request checklist

Uh oh!

rosahbruno commented Aug 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

badboy commented Aug 26, 2022

Uh oh!

badboy left a comment

Choose a reason for hiding this comment

Uh oh!

badboy commented Aug 26, 2022

Uh oh!

rosahbruno commented Aug 26, 2022

Uh oh!

badboy commented Aug 26, 2022

Uh oh!

rosahbruno commented Aug 26, 2022

Uh oh!

rosahbruno Aug 26, 2022

Choose a reason for hiding this comment

Uh oh!

rosahbruno commented Aug 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rosahbruno commented Aug 24, 2022 •

edited

Loading