pool timeouts/rich media cherry-picks #796

Merged
floatingghost merged 24 commits from pool-timeouts into develop 2024-06-12 17:08:07 +00:00
No description provided.
floatingghost added 18 commits 2024-06-09 17:47:51 +00:00
Rich Media parsing was previously handled on-demand with a 2 second HTTP request timeout and retained only in Cachex. Every time a Pleroma instance is restarted it will have to request and parse the data for each status with a URL detected. When fetching a batch of statuses they were processed in parallel to attempt to keep the maximum latency at 2 seconds, but often resulted in a timeline appearing to hang during loading due to a URL that could not be successfully reached. URLs which had images links that expire (Amazon AWS) were parsed and inserted with a TTL to ensure the image link would not break.

Rich Media data is now cached in the database and fetched asynchronously. Cachex is used as a read-through cache. When the data becomes available we stream an update to the clients. If the result is returned quickly the experience is almost seamless. Activities were already processed for their Rich Media data during ingestion to warm the cache, so users should not normally encounter the asynchronous loading of the Rich Media data.

Implementation notes:

- The async worker is a Task with a globally unique process name to prevent duplicate processing of the same URL
- The Task will attempt to fetch the data 3 times with increasing sleep time between attempts
- The HTTP request obeys the default HTTP request timeout value instead of 2 seconds
- URLs that cannot be successfully parsed due to an unexpected error receives a negative cache entry for 15 minutes
- URLs that fail with an expected error will receive a negative cache with no TTL
- Activities that have no detected URLs insert a nil value in the Cachex :scrubber_cache so we do not repeat parsing the object content with Floki every time the activity is rendered
- Expiring image URLs are handled with an Oban job
- There is no automatic cleanup of the Rich Media data in the database, but it is safe to delete at any time
- The post draft/preview feature makes the URL processing synchronous so the rendered post preview will have an accurate rendering

Overall performance of timelines and creating new posts which contain URLs is greatly improved.
Websites are increasingly getting more bloated with tricks like inlining content (e.g., CNN.com) which puts pages at or above 5MB. This value may still be too low.
Removed back in 2019

https://github.com/mastodon/mastodon/pull/11213
warning: "else" clauses will never match because all patterns in "with" will always match
  lib/pleroma/web/rich_media/parser/ttl/opengraph.ex:10
attempt to fix some tests
Some checks are pending
ci/woodpecker/push/build-amd64 Pipeline is pending
ci/woodpecker/push/build-arm64 Pipeline is pending
ci/woodpecker/push/docs Pipeline is pending
ci/woodpecker/push/lint Pipeline is pending
ci/woodpecker/push/test Pipeline is pending
ci/woodpecker/pr/build-amd64 Pipeline is pending
ci/woodpecker/pr/build-arm64 Pipeline is pending
ci/woodpecker/pr/docs Pipeline is pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
c65379afea
floatingghost added 1 commit 2024-06-09 17:52:21 +00:00
remove prints
Some checks are pending
ci/woodpecker/push/build-amd64 Pipeline is pending
ci/woodpecker/push/build-arm64 Pipeline is pending
ci/woodpecker/push/docs Pipeline is pending
ci/woodpecker/push/lint Pipeline is pending
ci/woodpecker/push/test Pipeline is pending
ci/woodpecker/pr/build-amd64 Pipeline is pending
ci/woodpecker/pr/build-arm64 Pipeline is pending
ci/woodpecker/pr/docs Pipeline is pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
840c70c4fa
Contributor

seems to be working so far on akko.wtf, memory usage is significantly lower than before

seems to be working so far on akko.wtf, memory usage is significantly lower than before
floatingghost added 2 commits 2024-06-09 20:26:42 +00:00
fix tests
Some checks failed
ci/woodpecker/push/build-amd64 Pipeline is pending
ci/woodpecker/push/build-arm64 Pipeline is pending
ci/woodpecker/push/docs Pipeline is pending
ci/woodpecker/push/lint Pipeline is pending
ci/woodpecker/push/test Pipeline is pending
ci/woodpecker/pr/lint Pipeline was successful
ci/woodpecker/pr/test Pipeline failed
ci/woodpecker/pr/build-amd64 unknown status
ci/woodpecker/pr/docs unknown status
ci/woodpecker/pr/build-arm64 unknown status
9c5feb81aa
floatingghost added 1 commit 2024-06-10 14:10:57 +00:00
add diagnostic script
Some checks are pending
ci/woodpecker/push/build-amd64 Pipeline is pending
ci/woodpecker/push/build-arm64 Pipeline is pending
ci/woodpecker/push/docs Pipeline is pending
ci/woodpecker/push/lint Pipeline is pending
ci/woodpecker/push/test Pipeline is pending
ci/woodpecker/pr/build-amd64 Pipeline is pending
ci/woodpecker/pr/build-arm64 Pipeline is pending
ci/woodpecker/pr/docs Pipeline is pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
28d357f52c
Member

There are two follow up PRs in Pleroma fixing some mistakes in the cherry-picked richmedia refactor:

Seem unrelated to the binary leaks we observed in testing though

There are two follow up PRs in Pleroma fixing some mistakes in the cherry-picked richmedia refactor: - https://git.pleroma.social/pleroma/pleroma/-/merge_requests/4121 - https://git.pleroma.social/pleroma/pleroma/-/merge_requests/4130 Seem unrelated to the binary leaks we observed in testing though
floatingghost added 1 commit 2024-06-11 17:07:07 +00:00
Convert rich media backfill to oban task
Some checks are pending
ci/woodpecker/push/build-amd64 Pipeline is pending
ci/woodpecker/push/build-arm64 Pipeline is pending
ci/woodpecker/push/docs Pipeline is pending
ci/woodpecker/push/lint Pipeline is pending
ci/woodpecker/push/test Pipeline is pending
ci/woodpecker/pr/build-amd64 Pipeline is pending
ci/woodpecker/pr/build-arm64 Pipeline is pending
ci/woodpecker/pr/docs Pipeline is pending
ci/woodpecker/pr/lint Pipeline is pending
ci/woodpecker/pr/test Pipeline is pending
ad52135bf5
Author
Owner

Async tasks we love async tasks don't we folks we love memory management it's the best

Async tasks we love async tasks don't we folks we love memory management it's the best
Oneric reviewed 2024-06-11 18:20:36 +00:00
@ -33,3 +34,1 @@
spawn(fn ->
Pleroma.Web.RichMedia.Helpers.fetch_data_for_activity(activity)
end)
spawn(fn -> Card.get_by_activity(activity) end)
Member

does this spawning still maeks sense now that updates can be streamed if it wasn't cached yet?

does this spawning still maeks sense now that updates can be streamed if it wasn't cached yet?
Author
Owner

nope it does not

especially now that it's just an oban

nope it does not especially now that it's just an oban
@ -0,0 +94,4 @@
end
@spec get_by_object(Object.t()) :: t() | nil | :error
def get_by_object(object) do
Member

where is this function used? Ctr+F doesn't show anything in the diff

where is this function used? `Ctr+F` doesn't show anything in the diff
Author
Owner

yeah seems nowhere

purged

yeah seems nowhere purged
@ -0,0 +10,4 @@
end
create(unique_index(:rich_media_card, [:url_hash]))
end
Member

url_hash might as well be the primary key here, considering it must be unique, we want fast lookups via it and afaict we don't ever care about the auto-generated numeric primary key.
Will save us space and an index for a unused primary key (but needs an extra migration to migrate to/from current Pleroma — ofc if PLeroma migrators are required to rollback to an earlier state before migration, this doesn't matter)

`url_hash` might as well be the primary key here, considering it must be unique, we want fast lookups via it and afaict we don't ever care about the auto-generated numeric primary key. Will save us space and an index for a unused primary key (but needs an extra migration to migrate to/from current Pleroma — ofc if PLeroma migrators are required to rollback to an earlier state before migration, this doesn't matter)
Author
Owner

i tried this, because :url_hash is a :bytea type, it cannot be used as a primary key
it seems ecto (and postgres in general) only really wants you to use either fixed-length :binary_id or integer :id fields as primary keys

i'll keep it as is - primary key indices shouldn't really be that much size-wise

i tried this, because :url_hash is a :bytea type, it cannot be used as a primary key it seems ecto (and postgres in general) only really wants you to use either fixed-length :binary_id or integer :id fields as primary keys i'll keep it as is - primary key indices shouldn't really be that much size-wise
Member

Plain PostgreSQL has no issue creating a table with a bytea primary key:

akkoma=> CREATE TABLE rich_media_card(url_hash bytea PRIMARY KEY, fields JSONB, created_at time, updated_at time);
CREATE TABLE
akkoma=> \d+ rich_media_card
                                             Table "public.rich_media_card"
   Column   |          Type          | Collation | Nullable | Default | Storage  | Compression | Stats target | Description 
------------+------------------------+-----------+----------+---------+----------+-------------+--------------+-------------
 url_hash   | bytea                  |           | not null |         | extended |             |              | 
 fields     | jsonb                  |           |          |         | extended |             |              | 
 created_at | time without time zone |           |          |         | plain    |             |              | 
 updated_at | time without time zone |           |          |         | plain    |             |              | 
Indexes:
    "rich_media_card_pkey" PRIMARY KEY, btree (url_hash)
Access method: heap
Plain PostgreSQL has no issue creating a table with a bytea primary key: ```sql akkoma=> CREATE TABLE rich_media_card(url_hash bytea PRIMARY KEY, fields JSONB, created_at time, updated_at time); CREATE TABLE akkoma=> \d+ rich_media_card Table "public.rich_media_card" Column | Type | Collation | Nullable | Default | Storage | Compression | Stats target | Description ------------+------------------------+-----------+----------+---------+----------+-------------+--------------+------------- url_hash | bytea | | not null | | extended | | | fields | jsonb | | | | extended | | | created_at | time without time zone | | | | plain | | | updated_at | time without time zone | | | | plain | | | Indexes: "rich_media_card_pkey" PRIMARY KEY, btree (url_hash) Access method: heap ```
Author
Owner

ecto won't allow it though 😩

ecto won't allow it though 😩
Member

Did the error only appear at runtime? Compiling and migrating with this works for me™:

defmodule Pleroma.Web.RichMedia.Card2 do
  use Ecto.Schema
  # ....

  @type t :: %__MODULE__{}
  # No implicit primary key, we explicitly define one below
  @primary_key false

  schema "rich_media_card2" do
    field(:url_hash, :binary, primary_key: true)
    field(:fields, :map)

    timestamps()
  end

# alternatively use
#   @primary_key {:url_hash, :binary, autogenerate: false}
#   and no url_hash entry in the schema block
defmodule Pleroma.Repo.Migrations.CreateRichMediaCard2 do
  use Ecto.Migration

  def change do
    create table(:rich_media_card2, primary_key: false) do
      add(:url_hash, :bytea, primary_key: true)
      add(:fields, :map)

      timestamps()
    end
  end
end

Also i only now noticed the migration ID is again out of order, leading to belated rollbacks; an extra table might not matter too much though

Did the error only appear at runtime? Compiling and migrating with this works for me™: ```elixir defmodule Pleroma.Web.RichMedia.Card2 do use Ecto.Schema # .... @type t :: %__MODULE__{} # No implicit primary key, we explicitly define one below @primary_key false schema "rich_media_card2" do field(:url_hash, :binary, primary_key: true) field(:fields, :map) timestamps() end # alternatively use # @primary_key {:url_hash, :binary, autogenerate: false} # and no url_hash entry in the schema block ``` ```elixir defmodule Pleroma.Repo.Migrations.CreateRichMediaCard2 do use Ecto.Migration def change do create table(:rich_media_card2, primary_key: false) do add(:url_hash, :bytea, primary_key: true) add(:fields, :map) timestamps() end end end ``` Also i only now noticed the migration ID is again out of order, leading to belated rollbacks; an extra table might not matter too much though
floatingghost added 1 commit 2024-06-12 01:10:21 +00:00
No need to spawn() any more
Some checks are pending
ci/woodpecker/push/build-amd64 Pipeline is pending
ci/woodpecker/push/build-arm64 Pipeline is pending
ci/woodpecker/push/docs Pipeline is pending
ci/woodpecker/push/lint Pipeline is pending
ci/woodpecker/push/test Pipeline is pending
ci/woodpecker/pr/lint Pipeline was successful
ci/woodpecker/pr/test Pipeline was successful
ci/woodpecker/pr/build-amd64 Pipeline was successful
ci/woodpecker/pr/build-arm64 Pipeline was successful
ci/woodpecker/pr/docs Pipeline was successful
4d6fb43cbd
Author
Owner

ship it (tm)

ship it (tm)
floatingghost changed title from WIP: pool timeouts/rich media cherry-picks to pool timeouts/rich media cherry-picks 2024-06-12 17:07:58 +00:00
floatingghost merged commit 5b75fb2a2f into develop 2024-06-12 17:08:07 +00:00
floatingghost deleted branch pool-timeouts 2024-06-12 17:08:07 +00:00
Sign in to join this conversation.
No description provided.