ref
https://linear.app/tryghost/issue/ENG-1543/debounce-the-members-lastseenatupdater
- The `LastSeenAtUpdater.updateLastSeenAt` function is called in
response to a `MemberClickEvent` — when a member clicks a link in an
email with tracking enabled. This function can be called many times for
the same member in a short period of time if e.g. a link checker is
clicking all the links in an email they received.
- This function should only update a member's `last_seen_at` timestamp
once per day. To accomplish this, `updateLastSeenAt` runs a
`select...for update` query to find the member's current `last_seen_at`
timestamp, and only updates the timestamp if the current `last_seen_at`
is before the start of the current day. The `for update` is required to
avoid a race condition, which previously caused this function to update
the `last_seen_at` timestamp more frequently than needed, which results
in many unnecessary database queries. However, we still run the initial
`select...for update` query for each event, which seems to be resulting
in contention for locks on the member's row in the `members` table.
- This commit introduces a simple in-memory cache so that we avoid
calling `updateLastSeenAt` if the member's `last_seen_at` timestamp has
already been updated in the current day, which should avoid running so
many `select...for update` queries and locking the `members` table up.
no issue
- These tests were all in a single `describe` block, making it difficult
to navigate and add more tests.
- This commit adds a nested `describe` block for each method on the
`lastSeenAtUpdater`, making it easier to understand the test cases, and
to add more. None of the tests themselves have changed, just the
organization of them.
ref https://linear.app/tryghost/issue/ENG-1266
- Mexico changed tz to not participate in DST
- our package was a couple years behind, so we likely have fixes for
other countries/regions, too
ref
https://linear.app/tryghost/issue/ENG-1240/race-condition-when-updating-members-last-seen-at-timestamp
When members click a link in an email, Ghost updates the member's
`last_seen_at` timestamp, but it should only update the timestamp if the
member hasn't yet been seen in the current day (based on the
publication's timezone).
Currently there is a race condition present where multiple simultaneous
requests from the same member (if e.g. an email link checker is
following all links in an email) can cause the `last_seen_at` timestamp
to be updated multiple times in the same day for the same member. These
additional queries add a significant load on Ghost and its database,
which can contribute to the exhaustion of the connection pool and
eventually requests may time out.
The primary motivation for this change is to avoid that race condition
by adding a lock to the member row, checking if `last_seen_at` has
already been updated in the current day, and only updating it if it
hasn't.
Another beneficial side-effect of this change is that it avoids locking
the `labels` and `newsletters` tables, which are locked when we update
the `last_seen_at` timestamp in the `members` table currently. This
should improve Ghost's ability to handle a large influx of requests to
redirect endpoints (confirmed with load tests), which tend to happen
immediately after a publisher sends an email.
ref https://linear.app/tryghost/issue/CFR-4/
- added request queueing middleware (express-queue) to handle high
request volume
- added new config option `optimization.requestQueue`
- added new config option `optimization.requestConcurrency`
- added logging of request queue depth - `req.queueDepth`
We've done a fair amount of investigation around improving Ghost's
resiliency to high request volume. While we believe this to be partly
due to database connection contention, it also seems Ghost gets
overwhelmed by the requests themselves. Implementing a simple queueing
system allows us a simple lever to change the volume of requests Ghost
is actually ingesting at any given time and gives us options besides
simply increasing database connection pool size.
---------
Co-authored-by: Michael Barrett <mike@ghost.org>
- this version is written in TS, but was published a few months ago and
needs to be bumped here
- also updates a previous deep include into the library, which was
unnecessary anyway
refs: https://github.com/TryGhost/Toolbox/issues/595
We're rolling out new rules around the node assert library, the first of which is enforcing the use of assert/strict. This means we don't need to use the strict version of methods, as the standard version will work that way by default.
This caught some gotchas in our existing usage of assert where the lack of strict mode had unexpected results:
- Url matching needs to be done on `url.href` see aa58b354a4
- Null and undefined are not the same thing, there were a few cases of this being confused
- Particularly questionable changes in [PostExporter tests](c1a468744b) tracked [here](https://github.com/TryGhost/Team/issues/3505).
- A typo see eaac9c293a
Moving forward, using assert strict should help us to catch unexpected behaviour, particularly around nulls and undefineds during implementation.
refs: https://github.com/TryGhost/Toolbox/issues/188
- some of our older packages used a pattern for linting which missed using test config for linting tests
- we need this to be consistent so that we can add more eslint rules for testing
- two packages also didn't use the lib pattern, which made the lint pattern error - so this was fixed as well
As discussed with the product team we want to enforce kebab-case file names for
all files, with the exception of files which export a single class, in which
case they should be PascalCase and reflect the class which they export.
This will help find classes faster, and should push better naming for them too.
Some files and packages have been excluded from this linting, specifically when
a library or framework depends on the naming of a file for the functionality
e.g. Ember, knex-migrator, adapter-manager
- we previously used `@stdlib/utils` instead of the child package
`@stdlib/copy`, which is a lot smaller and contains our only use of
the parent
- this saves 140+MB of dependencies
- we keep ending up with multiple versions of the depedency in our tree,
and it's causing problems when comparing instances
- the workaround I'm implementing for now is to bump the package
everywhere and set a resolution so we only have 1 shared instance
- hopefully we can come up with a better method down the line
- there's a weird situation when we have mixed versions of the
dependency because different libraries try to compare instances
- this brings the usage up to 1.2.21 so we can fix the build for now
refs https://github.com/TryGhost/Toolbox/issues/501
- this reverts commit 48dda23554
- also includes a resolution for `@elastic/elasticsearch` so we don't
run a version that is potentially problematic - see referenced issue
for context
no issue
With the increased usage of DomainEvents, it gets harder to build
reliable tests without having to resort to timeouts. This utility method
allows us to wait for all events to be processed before continuing with
the test.
This change should speed up tests and make them more reliable.
It only adds extra code when running tests and shouldn't impact
production.
- this was all getting terribly behind so I've done several things:
- majority of `@tryghost/*` except Lexical packages
- gscan + knex-migrator to remove old `@tryghost/errors` usage
- bumped lockfile
refs https://ghost.slack.com/archives/C02G9E68C/p1670960248186789
This reverts a change that was made here:
f4fdb4fa6c (r93071549),
but it still moved the original code to a new location in the
LastSeenAtUpdater
It includes a new E2E test to make sure timezones are supported
correctly.
- By not using Bookshelf, we no longer fire webhook calls
- By not using the member repository, we don't fetch and update the
member model and the labels relation in a forUpdate transaction, which
caused deadlock issues on the labels/members_labels tables which were
hard to resolve. Until now I was unable to find the other conflicting
transaction that caused this deadlock. Moving to raw knex (instead of
Bookshelf) and only updating the last_updated_at column should remove
the deadlock issue.
This removed the test for the email service wrapper, since it started
failing for an unknown reason and the test didn't make much sense (was
added earlier only to bump test threshold).
refs https://ghost.slack.com/archives/C02G9E68C/p1670215917451249
When a member is deleted, and we receive an opened event for an email to
that member. We threw an uncaught Bookshelf EmptyResponse error.
- This change makes fetching the member not a requirement when handling
that event in the last seen at updater.
- It also adds try catches for all event listeners in the last seen at
updater
fixes https://github.com/TryGhost/Team/issues/2310
This moves the processing of the events from the event-processor to a
new email-event-processor in the email-service package.
- The `EmailEventProcessor` only translates events from
providerId/emailId to their known emailId, memberId and recipientId, and
dispatches the corresponding events.
- Since `EmailEventProcessor` runs in a separate worker thread, we can't
listen for the dispatched events on the main thread. To accomplish this
communication, the events dispatched from the `EmailEventProcessor`
class are 'posted' via the postMessage method and redispatched on the
main thread.
- A new `EmailEventStorage` class reacts to the email events and stores
it in the database. This code mostly corresponds to the (now deleted)
subclass of the old `EmailEventProcessor`
- Updating a members last_seen_at timestamp has moved to the
lastSeenAtUpdater.
- Email events no longer store `ObjectID` because these are not
encodable across threads via postMessage
- Includes new E2E tests that test the storage of all supported Mailgun
events. Note that in these tests we run the processing on the main
thread instead of on a separate thread (couldn't do this because
stubbing is not possible across threads)
There are some missing pieces that will get added in later PRs (this PR
focuses on porting the existing functionality):
- Handling temporary failures/bounces
- Capturing the error messages of bounce events
- renames `refSource`, `refMedium` and `refUrl` to `referrerSource`, `referrerMedium` and `referrerUrl` respectively for consistent naming across files and usages
fixes https://github.com/TryGhost/Team/issues/1952
Adds a new MemberLinkClickEvent event that is fired when a member clicks a link. This code has been added to the `linkClickRepository` because that is the only place that has access to the member model (and the event requires the id and current last seen at value). The LastSeenAtUpdater listens for this event and updates the timestamp if required.
fixes https://github.com/TryGhost/Team/issues/1821
This change moves all the event storage logic to one new place: the event storage class in the MembersEventsService, which is initialised in a new members events service wrapper.
Apart from this, this includes some improvements:
- Removed DomainEvents from the constructor arguments to the subscribe method (to make it more clear where to subscribe to and decrease dependencies)
- LastSeenAtUpdater doesn't subscribe in the constructor any longer (removes unclear side effect)
- Moved LastSeenAtUpdater initialisation to new members events service wrapper
- Added missing tests to LastSeenAtUpdater to assure that the MembersEventsService package has 100% coverage.
- cleaned up unused dependencies
- adds missing dependencies that are used in the code
- this should help us be more explicit about the dependencies a package
uses
- because of how the npm scripts were set up, we were running the full
Admin integration tests during the unit tests phase of CI
- this commit renames the majority of `test` to `test:unit` in the
package.json files, and aliases `test` to `test:unit`
- special packages like Admin have no-op'd `test:unit` scripts so we
don't end up running its tests
