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.

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

Only sync Glean.js #1808

Merged
merged 37 commits into from
Nov 20, 2023
Merged

Only sync Glean.js #1808

merged 37 commits into from
Nov 20, 2023

Conversation

rosahbruno
Copy link
Contributor

@rosahbruno rosahbruno commented Nov 8, 2023

This PR updates Glean.js to be completely synchronous with only a web implementation.

The 3 other platforms are going to undergo changes

Both the node and web extension platforms are going to be added back in later. They are being removed now so we can focus on web and build a new architecture that allows for platform OR framework specific implementations with their own auto-instrumentation.

Important updates included in this PR

  • Removal of async/sync file structure
  • Removal of the dispatcher since it is not needed in the sync implementation
  • Removal of plugins as they are asynchronous and not used in any active projects
  • Removal of tests that are async specific and no longer fit in Glean.js

@Dexterp37
Copy link
Contributor

Dexterp37 commented Nov 8, 2023

@rosahbruno a few observations on this:

  • Please remove the "builtin metrics" from this PR. We should tackle this as a follow-up after this refactor.
  • Please break out the QT code removal out of this PR, to simplify reviewing (instead of removing the docs from the Glean Book, we should mark that as deprecated, mention the last supported version and file a bug for moving these docs to an "Obsolete" part of the book) - Update: Took a stab at this in Remove QT #1810

Note that I see 13 unit tests failing locally, not 2:

  • Some of them are failing because of isWindowObjectUnavailable in internal_metrics.ts/initialize
  • Some of them are failing because of the newly automatically instrumented page load events (e.g. "send the 'events' ping when max capacity is hit")

By splitting the PR as advised above we should be more confident in the changes we make.

@rosahbruno
Copy link
Contributor Author

@rosahbruno a few observations on this:

* Please remove the "builtin metrics" from this PR. We should tackle this as a follow-up after this refactor.

* Please break out the QT code removal out of this PR, to simplify reviewing (instead of removing the docs from the Glean Book, we should mark that as deprecated, mention the last supported version and file a bug for moving these docs to an "Obsolete" part of the book)

Yep, this makes sense. I will break these into separate PRs.

Note that I see 13 unit tests failing locally, not 2:

* Some of them are failing because of `isWindowObjectUnavailable` in internal_metrics.ts/initialize

* Some of them are failing because of the newly automatically instrumented page load events (e.g. `"send the 'events' ping when max capacity is hit"`)

By splitting the PR as advised above we should be more confident in the changes we make.

I will take a look

@rosahbruno
Copy link
Contributor Author

rosahbruno commented Nov 10, 2023

This PR is now rebased and includes the changes from #1810. It also includes and builds off the changes made in #1813 and #1814.

I am going to clean up these commits so they are more readable and easier to review.

@rosahbruno rosahbruno marked this pull request as ready for review November 17, 2023 18:55
@auto-assign auto-assign bot requested a review from travis79 November 17, 2023 18:56
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.

No blocking comments or arguments from me. That was a lot of await and async to remove!

glean/src/core/metrics/database.ts Outdated Show resolved Hide resolved
glean/src/core/error/error_type.ts Outdated Show resolved Hide resolved
rosahbruno and others added 2 commits November 20, 2023 16:13
@rosahbruno rosahbruno merged commit e7ebd1f into mozilla:main Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants