-
Notifications
You must be signed in to change notification settings - Fork 3.5k
feat(tasks): download large living artifact files through presigned urls #110967
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1028,3 +1028,9 @@ class LivingArtifactVersionContent: | |
| name: str | ||
| content_type: str | ||
| content: bytes | ||
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win Use the repository dataclass decorator. Replace Proposed decorator change-@dataclass(frozen=True, kw_only=True)
+@frozenSource: Coding guidelines |
||
| class LivingArtifactVersionDownload: | ||
| url: str | None | ||
| error: Literal["not_found", "not_stored", "unavailable"] | None | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import structlog | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from slack_sdk.errors import SlackApiError | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from posthog.dataclasses import frozen | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from posthog.event_usage import groups | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from posthog.ph_client import ph_scoped_capture | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from posthog.slack.channels import MAX_BUTTON_URL_CHARS, SlackButton, section_block | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -377,12 +378,25 @@ def open_task_artifact(artifact: TaskArtifact) -> str | None: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return _adapter_for_existing_artifact(artifact).open(artifact) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def read_living_artifact_version(artifact: TaskArtifact, version: int) -> LivingArtifactVersionContent | None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Return the content of one version, or None when the version is unknown or keeps no content. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # The app streams a preview through a web worker, so a larger stored version only downloads. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Keep in step with LIVING_PREVIEW_MAX_BYTES in the TaskTracker frontend. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| LIVING_VERSION_PREVIEW_MAX_BYTES = 25 * 1024 * 1024 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| A Slack file version keeps its bytes in object storage. A canvas or message version keeps its | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| text in the version record. Storage read errors propagate to the caller. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class LivingArtifactVersionTooLarge(Exception): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pass | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @frozen | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class LivingVersionLocation: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| record: dict[str, Any] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| content_type: str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Empty when the version keeps its content as text in the record. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| storage_path: str | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def resolve_living_artifact_version(artifact: TaskArtifact, version: int) -> LivingVersionLocation | None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Find one version and where it keeps its content, or None when the version is unknown or its path is foreign.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| record = next( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| candidate | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -398,20 +412,47 @@ def read_living_artifact_version(artifact: TaskArtifact, version: int) -> Living | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| content_type = str(record.get("content_type") or location.get("content_type") or "") or _guess_content_type( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| artifact.name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| storage_path = str(location.get("storage_path") or "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if storage_path: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Every living artifact object sits under its task's prefix. A path outside it is not this artifact's object. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not storage_path.startswith(_task_artifact_s3_prefix(artifact)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| payload = object_storage.read_bytes(storage_path, missing_ok=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Every living artifact object sits under its task's prefix. A path outside it is not this artifact's object. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if storage_path and not storage_path.startswith(_task_artifact_s3_prefix(artifact)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return LivingVersionLocation(record=record, content_type=content_type, storage_path=storage_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _stored_version_size(resolved: LivingVersionLocation) -> int | None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| size = resolved.record.get("size") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(size, int): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return size | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| head = object_storage.head_object(resolved.storage_path) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| length = head.get("ContentLength") if head else None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return length if isinstance(length, int) else None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def read_living_artifact_version(artifact: TaskArtifact, version: int) -> LivingArtifactVersionContent | None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Return the content of one version, or None when the version is unknown or keeps no content. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| A Slack file version keeps its bytes in object storage. A canvas or message version keeps its | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| text in the version record. A stored version above the preview limit raises | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| LivingArtifactVersionTooLarge. Storage read errors propagate to the caller. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolved = resolve_living_artifact_version(artifact, version) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if resolved is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if resolved.storage_path: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| size = _stored_version_size(resolved) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if size is not None and size > LIVING_VERSION_PREVIEW_MAX_BYTES: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise LivingArtifactVersionTooLarge() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| payload = object_storage.read_bytes(resolved.storage_path, missing_ok=True) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if payload is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return LivingArtifactVersionContent(name=artifact.name, content_type=content_type, content=payload) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return LivingArtifactVersionContent(name=artifact.name, content_type=resolved.content_type, content=payload) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+431
to
+449
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '388,465p' products/tasks/backend/logic/services/living_artifacts.py
rg -n 'def head_object|def read_bytes' posthog/storage/object_storage.pyRepository: PostHog/posthog Length of output: 4464 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- object storage module slices ---'
sed -n '1,90p' posthog/storage/object_storage.py
sed -n '560,785p' posthog/storage/object_storage.py
printf '%s\n' '--- reader usages and endpoint context ---'
rg -n -C 5 'read_living_artifact_version|LivingArtifactVersionTooLarge|living_artifact_version_storage_path' products/tasks posthog | head -240
printf '%s\n' '--- relevant route/view names ---'
rg -n -C 4 'preview|living_artifact|artifact.*version|version.*artifact' products/tasks/backend products/tasks/frontend 2>/dev/null | head -260Repository: PostHog/posthog Length of output: 41511 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- object-storage read_bytes implementations ---'
sed -n '100,175p' posthog/storage/object_storage.py
sed -n '180,215p' posthog/storage/object_storage.py
sed -n '280,315p' posthog/storage/object_storage.py
printf '%s\n' '--- preview facade functions ---'
sed -n '4345,4465p' products/tasks/backend/facade/api.py
printf '%s\n' '--- callers and route registration ---'
rg -n -C 6 'get_living_artifact_version_content|get_living_artifact_version_url|living_artifact_version_content|living_artifact_version_url' products/tasks --glob '*.py'Repository: PostHog/posthog Length of output: 13071 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- exact facade callers and route bindings ---'
rg -n -C 5 'read_task_run_living_artifact_version' products/tasks --glob '*.py'
printf '%s\n' '--- preview endpoint test ---'
sed -n '8585,8705p' products/tasks/backend/tests/test_api.pyRepository: PostHog/posthog Length of output: 9071 Enforce the preview limit when storage size is unknown. When both the version record and Suggested fix size = _stored_version_size(resolved)
- if size is not None and size > LIVING_VERSION_PREVIEW_MAX_BYTES:
+ if size is None or size > LIVING_VERSION_PREVIEW_MAX_BYTES:
raise LivingArtifactVersionTooLarge()📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| text = record.get("content") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| text = resolved.record.get("content") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(text, str): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return LivingArtifactVersionContent(name=artifact.name, content_type=content_type, content=text.encode("utf-8")) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return LivingArtifactVersionContent( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| name=artifact.name, content_type=resolved.content_type, content=text.encode("utf-8") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PostHog/posthog
Length of output: 10234
🏁 Script executed:
Repository: PostHog/posthog
Length of output: 41990
🏁 Script executed:
Repository: PostHog/posthog
Length of output: 16642
Move the resolver import to module scope.
The local import violates the backend import rule.
living_artifacts.pyimportsobject_storage, but this does not eagerly initialize a storage client:object_storage.pystarts withUnavailableStorage, and imports boto3 only whenobject_storage_client()is called. Do not split the module solely to avoid eager storage initialization.