Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add tracing to media /upload endpoint #15850

Merged
merged 3 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15850.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add tracing to media `/upload` code paths.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

My only question is whether it's harmless to leave these indefinitely after the investigation has concluded or if there needs to be a future maintenance milestone to remove them. Probably not a big deal either way though.

-- #15850 (review)

I think we can leave them in forever. They're useful to look into any problem even retroactively if you want to investigate something. It's something, I wish our codebase just had automatically without having to manually instrument everything.

Performance-wise, they're a no-op unless tracing is enabled and your user is sampled.

3 changes: 3 additions & 0 deletions synapse/media/media_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
from synapse.config.repository import ThumbnailRequirement
from synapse.http.site import SynapseRequest
from synapse.logging.context import defer_to_thread
from synapse.logging.opentracing import trace
from synapse.media._base import (
FileInfo,
Responder,
Expand Down Expand Up @@ -174,6 +175,7 @@ def mark_recently_accessed(self, server_name: Optional[str], media_id: str) -> N
else:
self.recently_accessed_locals.add(media_id)

@trace
async def create_content(
self,
media_type: str,
Expand Down Expand Up @@ -710,6 +712,7 @@ async def generate_remote_exact_thumbnail(
# Could not generate thumbnail.
return None

@trace
async def _generate_thumbnails(
self,
server_name: Optional[str],
Expand Down
7 changes: 7 additions & 0 deletions synapse/media/media_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@

from synapse.api.errors import NotFoundError
from synapse.logging.context import defer_to_thread, make_deferred_yieldable
from synapse.logging.opentracing import trace
from synapse.util import Clock
from synapse.util.file_consumer import BackgroundFileConsumer

Expand Down Expand Up @@ -76,6 +77,7 @@ def __init__(
self._spam_checker_module_callbacks = hs.get_module_api_callbacks().spam_checker
self.clock = hs.get_clock()

@trace
async def store_file(self, source: IO, file_info: FileInfo) -> str:
"""Write `source` to the on disk media store, and also any other
configured storage providers
Expand All @@ -95,10 +97,12 @@ async def store_file(self, source: IO, file_info: FileInfo) -> str:

return fname

@trace
async def write_to_file(self, source: IO, output: IO) -> None:
"""Asynchronously write the `source` to `output`."""
await defer_to_thread(self.reactor, _write_file_synchronously, source, output)

@trace
@contextlib.contextmanager
def store_into_file(
MadLittleMods marked this conversation as resolved.
Show resolved Hide resolved
self, file_info: FileInfo
Expand Down Expand Up @@ -214,6 +218,7 @@ async def fetch_media(self, file_info: FileInfo) -> Optional[Responder]:

return None

@trace
async def ensure_media_is_in_local_cache(self, file_info: FileInfo) -> str:
"""Ensures that the given file is in the local cache. Attempts to
download it from storage providers if it isn't.
Expand Down Expand Up @@ -259,6 +264,7 @@ async def ensure_media_is_in_local_cache(self, file_info: FileInfo) -> str:

raise NotFoundError()

@trace
def _file_info_to_path(self, file_info: FileInfo) -> str:
"""Converts file_info into a relative path.

Expand Down Expand Up @@ -301,6 +307,7 @@ def _file_info_to_path(self, file_info: FileInfo) -> str:
return self.filepaths.local_media_filepath_rel(file_info.file_id)


@trace
def _write_file_synchronously(source: IO, dest: IO) -> None:
"""Write `source` to the file like `dest` synchronously. Should be called
from a thread.
Expand Down
5 changes: 5 additions & 0 deletions synapse/media/storage_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

from synapse.config._base import Config
from synapse.logging.context import defer_to_thread, run_in_background
from synapse.logging.opentracing import trace
from synapse.util.async_helpers import maybe_awaitable

from ._base import FileInfo, Responder
Expand Down Expand Up @@ -86,6 +87,7 @@ def __init__(
def __str__(self) -> str:
return "StorageProviderWrapper[%s]" % (self.backend,)

@trace
async def store_file(self, path: str, file_info: FileInfo) -> None:
if not file_info.server_name and not self.store_local:
return None
Expand Down Expand Up @@ -114,6 +116,7 @@ async def store() -> None:

run_in_background(store)

@trace
async def fetch(self, path: str, file_info: FileInfo) -> Optional[Responder]:
if file_info.url_cache:
# Files in the URL preview cache definitely aren't stored here,
Expand Down Expand Up @@ -141,6 +144,7 @@ def __init__(self, hs: "HomeServer", config: str):
def __str__(self) -> str:
return "FileStorageProviderBackend[%s]" % (self.base_directory,)

@trace
async def store_file(self, path: str, file_info: FileInfo) -> None:
"""See StorageProvider.store_file"""

Expand All @@ -159,6 +163,7 @@ async def store_file(self, path: str, file_info: FileInfo) -> None:
backup_fname,
)

@trace
async def fetch(self, path: str, file_info: FileInfo) -> Optional[Responder]:
"""See StorageProvider.fetch"""

Expand Down
5 changes: 5 additions & 0 deletions synapse/media/thumbnailer.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

from PIL import Image

from synapse.logging.opentracing import trace

logger = logging.getLogger(__name__)

EXIF_ORIENTATION_TAG = 0x0112
Expand Down Expand Up @@ -82,6 +84,7 @@ def __init__(self, input_path: str):
# A lot of parsing errors can happen when parsing EXIF
logger.info("Error parsing image EXIF information: %s", e)

@trace
def transpose(self) -> Tuple[int, int]:
"""Transpose the image using its EXIF Orientation tag

Expand Down Expand Up @@ -133,6 +136,7 @@ def _resize(self, width: int, height: int) -> Image.Image:
self.image = self.image.convert("RGB")
return self.image.resize((width, height), Image.ANTIALIAS)

@trace
def scale(self, width: int, height: int, output_type: str) -> BytesIO:
"""Rescales the image to the given dimensions.

Expand All @@ -142,6 +146,7 @@ def scale(self, width: int, height: int, output_type: str) -> BytesIO:
with self._resize(width, height) as scaled:
return self._encode_image(scaled, output_type)

@trace
def crop(self, width: int, height: int, output_type: str) -> BytesIO:
"""Rescales and crops the image to the given dimensions preserving
aspect::
Expand Down
1 change: 1 addition & 0 deletions synapse/module_api/callbacks/spamchecker_callbacks.py
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,7 @@ async def check_registration_for_spam(

return RegistrationBehaviour.ALLOW

@trace
async def check_media_file_for_spam(
self, file_wrapper: ReadableFileWrapper, file_info: FileInfo
) -> Union[Tuple[Codes, dict], Literal["NOT_SPAM"]]:
Expand Down
5 changes: 5 additions & 0 deletions synapse/storage/databases/main/media_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
)

from synapse.api.constants import Direction
from synapse.logging.opentracing import trace
from synapse.storage._base import SQLBaseStore
from synapse.storage.database import (
DatabasePool,
Expand Down Expand Up @@ -328,6 +329,7 @@ def _get_local_media_ids_txn(txn: LoggingTransaction) -> List[str]:
"get_local_media_ids", _get_local_media_ids_txn
)

@trace
async def store_local_media(
self,
media_id: str,
Expand Down Expand Up @@ -447,6 +449,7 @@ async def get_local_media_thumbnails(self, media_id: str) -> List[Dict[str, Any]
desc="get_local_media_thumbnails",
)

@trace
async def store_local_thumbnail(
self,
media_id: str,
Expand Down Expand Up @@ -568,6 +571,7 @@ async def get_remote_media_thumbnails(
desc="get_remote_media_thumbnails",
)

@trace
async def get_remote_media_thumbnail(
self,
origin: str,
Expand Down Expand Up @@ -599,6 +603,7 @@ async def get_remote_media_thumbnail(
desc="get_remote_media_thumbnail",
)

@trace
async def store_remote_media_thumbnail(
self,
origin: str,
Expand Down