Skip to content

Add video playback observability to Sentry#11111

Open
vineyardbovines wants to merge 1 commit into
mainfrom
app-2585
Open

Add video playback observability to Sentry#11111
vineyardbovines wants to merge 1 commit into
mainfrom
app-2585

Conversation

@vineyardbovines

Copy link
Copy Markdown
Member

Adds Sentry performance spans around video playback in feeds, to establish old-arch baselines for scroll + autoplay performance ahead of the new architecture upgrade.

What

  • Enables tracesSampleRate (1.0 internal, 1% prod). This is what makes spans send at all, and it auto-enables the SDK's JS stall tracking and slow/frozen frame tracking, whose measurements attach to every root span. Also adds a new_arch tag for filtering.
  • New createPlaybackTelemetry (src/lib/media/video/playbackTelemetry.ts): opens one video.playback root span per activation window (video becomes the active autoplaying one, ends on deactivation) with surface, presentation, timeToReadyMs, timeToFirstPlayMs, outcome, and error info. Since these are root spans, stall and frame measurements ride along, capturing scroll smoothness while a video is on screen. Sentry-only; nothing goes to the analytics pipeline.
  • Wires it into both native surfaces:
    • in-feed embeds, via the BlueskyVideoView event callbacks in VideoEmbed/index.tsx (plus an optional onError prop on VideoEmbedInnerNative)
    • the immersive video feed, via a usePlaybackTelemetry hook on the expo-video players, with a preloaded attribute to separate load time from swipe latency

Web is untouched. Side effect: the existing video.upload spans, which were created but never sampled, now flow at the same rates.

Test plan

  • Unit tests for the telemetry module (span lifecycle, idempotent end, error outcome, re-activation).
  • pnpm typecheck, pnpm lint, pnpm test, pnpm prettier pass.
  • Runtime spans verified via the next internal build (Sentry is disabled in dev).

@linear-code

linear-code Bot commented Jul 8, 2026

Copy link
Copy Markdown

APP-2585

@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @vineyardbovines's task in 6m 16s —— View job


Review: Add video playback observability to Sentry

  • Read review guidance & CLAUDE.md
  • Read changed files & diff
  • Analyze telemetry module, Sentry setup, and wiring
  • Post review

Solid, well-scoped PR. The createPlaybackTelemetry module is clean, idempotent, and has good unit coverage (span lifecycle, dup activation, once-only ready/playing, error vs ok outcome, re-activation). The lazy ??= creation keeps videos that never activate free, and the wiring on both native surfaces (VideoEmbed/index.tsx, usePlaybackTelemetry in VideoFeed) correctly pairs activated/deactivated via effect cleanup. No crashes, moderation regressions, or re-render storms introduced. A couple of things worth confirming, both non-blocking.


1. tracesSampleRate also turns on tracing for web, which partially contradicts "web is untouched" — non-blocking, worth confirming

src/logger/sentry/setup/index.ts:38 is the single shared Sentry init (no .web.ts variant — confirmed setup/ only contains index.ts, imported by both App.tsx and App.web.tsx). So tracesSampleRate: 0.01 in prod now enables performance tracing on all platforms, web included.

On @sentry/react-native, going from tracesSampleRate: undefined (tracing off) to a non-zero value activates the SDK's default tracing behavior. On native that's the intended stall/frame tracking; on web the underlying browser SDK's automatic instrumentation (pageload/navigation transactions, and depending on default integrations, fetch/xhr spans) can start emitting transactions you weren't sending before. The PR body frames web as untouched and only calls out video.upload spans now flowing — but this is a broader web-transaction-volume (and Sentry quota) change than that note implies.

Worth confirming this is intended, or gating the rate behind a platform check if web tracing isn't wanted yet:

tracesSampleRate: IS_WEB ? 0 : (env.IS_INTERNAL ? 1.0 : 0.01),

Investigate web tracing impact →

2. In-feed embed can drop timeToReadyMs if loading finishes before activation — non-blocking data-quality note

In src/components/Post/Embed/VideoEmbed/index.tsx:113-130, ready() fires from onLoadingChange and the telemetry object is only created on onActiveChange(true). onLoadingChange and onActiveChange are independent native events. If a loading -> not loading transition lands before active becomes true, telemetryRef.current?.ready() no-ops against a null ref, and since activated() resets sawReady = false without re-checking load state, timeToReadyMs may never be recorded for that window. For scroll-driven autoplay, activation typically precedes load so this is likely rare, but it means timeToReadyMs can be silently absent on some in-feed spans. Not a correctness bug — just a gap to be aware of when reading the dashboards. (The immersive hook avoids this by seeding ready() from player.status === 'readyToPlay' at activation.)


Everything else checks out: String(e).slice(0, 256) guards error size, getGlobalScope().setTag('new_arch', ...) runs after init() and resolves to false on web harmlessly, and the previously-unsampled video.upload spans now flowing is correctly called out. The optional onError prop on VideoEmbedInnerNative is additive and doesn't change existing error-boundary behavior.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor
Old size New size Diff
7.87 MB 7.87 MB 111 B (0.00%)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds Sentry performance tracing around video playback so the app can capture “scroll + autoplay” performance baselines (including JS stall and slow/frozen frame measurements) ahead of the new-architecture upgrade.

Changes:

  • Enables Sentry tracesSampleRate (1.0 internal / 1% prod) and adds a new_arch tag on the global scope for filtering.
  • Introduces createPlaybackTelemetry to manage a root video.playback span per “activation window”, recording readiness/play timing and outcomes.
  • Wires playback telemetry into the immersive video feed (expo-video players) and in-feed native video embeds (BlueskyVideoView callbacks), including error reporting and unit tests.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/screens/VideoFeed/index.tsx Adds a usePlaybackTelemetry hook to open/close playback spans for the active immersive-feed player, including a preloaded signal.
src/logger/sentry/setup/index.ts Enables performance tracing sampling and sets a new_arch tag on the global Sentry scope for easier filtering.
src/lib/media/video/playbackTelemetry.ts New telemetry helper that manages the playback span lifecycle and attaches timing/outcome attributes.
src/lib/media/video/tests/playbackTelemetry.test.ts Adds unit tests for span lifecycle behavior (activation, idempotent end, error outcome, re-activation).
src/components/Post/Embed/VideoEmbed/VideoEmbedInner/VideoEmbedInnerNative.tsx Adds an optional onError callback so callers can record telemetry before throwing to the error boundary.
src/components/Post/Embed/VideoEmbed/index.tsx Hooks telemetry into in-feed video embed state transitions (active/loading/status) and forwards native errors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@estrattonbailey estrattonbailey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment on lines +121 to +129
if (active) {
telemetryRef.current ??= createPlaybackTelemetry({
surface: 'feed',
presentation: embed.presentation === 'gif' ? 'gif' : 'video',
})
telemetryRef.current.activated()
} else {
telemetryRef.current?.deactivated()
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So if it becomes active again, will we re-open the span or start a new one, or can it only report telemetry once in it's lifecycle? Thinking about a user like scrolling back up to a previous video.

Comment on lines +46 to +50
if (span) return
activatedAt = Date.now()
sawReady = false
sawPlaying = false
span = Sentry.startInactiveSpan({

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok looks like it will create a new one, nice

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.

4 participants