feat: add telemetry component#10
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the client-side observability of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new telemetry component for client-side metrics, leveraging OpenTelemetry. The changes involve refactoring the existing observability logic into a new zero-dependency bridge and a web component, enhancing modularity and flexibility. The integration seems well-thought-out, with event buffering and dynamic SDK loading to minimize overhead. Additionally, the CORS configuration for the OTLP collector has been tightened, improving security. Overall, this is a significant and positive enhancement to the project's observability capabilities.
| type BytesEntry = { bytes: number; trackType: "video" | "audio"; attrs: Record<string, string> | undefined }; | ||
|
|
||
| // Safety cap: avoid unbounded growth if many connections happen before a provider loads. | ||
| const MAX_PENDING = 50; |
There was a problem hiding this comment.
The MAX_PENDING constant is a magic number. While its purpose is explained in the comments, consider making this value configurable, perhaps through an environment variable or a parameter, if there's a scenario where different buffering limits might be desired. This would improve flexibility without hardcoding a potentially arbitrary limit.
| recordConnection(transport: "webtransport" | "websocket", attrs?: Record<string, string>): void; | ||
|
|
||
| /** Called when the first frame is decoded. `ms` is time since playback started. */ | ||
| recordStartupTime(ms: number, attrs?: Record<string, string>): void; |
There was a problem hiding this comment.
The recordStartupTime in the TelemetryProvider interface and its implementation in bridge.ts expects ms (milliseconds). However, the corresponding metric moq_client_startup_time_seconds (defined in element.ts) implies a unit of seconds, and the element.ts implementation converts milliseconds to seconds before recording. To maintain consistency and clarity, it would be better if the TelemetryProvider interface and the recordStartupTime function in bridge.ts also expected seconds, aligning with the metric's unit.
Alternatively, if milliseconds are truly intended for the bridge, the metric name should reflect that (e.g., moq_client_startup_time_milliseconds). Given the current metric name, expecting seconds seems more appropriate.
| recordStartupTime(ms: number, attrs?: Record<string, string>): void; | |
| recordStartupTime(seconds: number, attrs?: Record<string, string>): void; |
| } | ||
|
|
||
| /** Record time-to-first-frame. `ms` is milliseconds from subscription start to first output. */ | ||
| export function recordStartupTime(ms: number, attrs?: Record<string, string>): void { |
There was a problem hiding this comment.
Following the feedback for the TelemetryProvider interface, this function should also accept seconds instead of ms for consistency with the metric unit. The conversion from milliseconds to seconds should ideally happen at the call site in audio/source.ts and video/source.ts if the source provides milliseconds, or the source should directly provide seconds.
| export function recordStartupTime(ms: number, attrs?: Record<string, string>): void { | |
| export function recordStartupTime(seconds: number, attrs?: Record<string, string>): void { |
| tryFlush(); | ||
| }, | ||
| recordStartupTime(ms, attrs) { | ||
| startupTime.record(ms / 1000, attrs); |
There was a problem hiding this comment.
Given the moq_client_startup_time_seconds metric name, it's appropriate to record in seconds. However, if the recordStartupTime function in bridge.ts is updated to accept seconds directly (as suggested in bridge.ts), then this division by 1000 would no longer be necessary here. The ms parameter would already be in seconds.
| startupTime.record(ms / 1000, attrs); | |
| startupTime.record(ms, attrs); |
| firstFrameDecoded = true; | ||
| const ttfaSeconds = (performance.now() - trackStartTime) / 1000; | ||
| recordMetric((m) => m.recordStartupTime(ttfaSeconds, { codec: config.codec, track_type: "audio" })); | ||
| recordStartupTime(ttfaSeconds * 1000, { codec: config.codec, track_type: "audio" }); |
There was a problem hiding this comment.
If recordStartupTime in telemetry/bridge.ts is updated to accept seconds (as suggested), then ttfaSeconds should be passed directly here, without multiplying by 1000. This ensures consistency across the telemetry system where the metric unit is seconds.
| recordStartupTime(ttfaSeconds * 1000, { codec: config.codec, track_type: "audio" }); | |
| recordStartupTime(ttfaSeconds, { codec: config.codec, track_type: "audio" }); |
| firstFrameRendered = true; | ||
| const ttffSeconds = (performance.now() - trackStartTime) / 1000; | ||
| recordMetric((m) => m.recordStartupTime(ttffSeconds, { codec: config.codec, track_type: "video" })); | ||
| recordStartupTime(ttffSeconds * 1000, { codec: config.codec, track_type: "video" }); |
There was a problem hiding this comment.
Similar to the audio source, if recordStartupTime in telemetry/bridge.ts is updated to accept seconds, then ttffSeconds should be passed directly here, without multiplying by 1000. This maintains consistency with the metric's unit.
| recordStartupTime(ttffSeconds * 1000, { codec: config.codec, track_type: "video" }); | |
| recordStartupTime(ttffSeconds, { codec: config.codec, track_type: "video" }); |
| - "http://localhost:5173" | ||
| allowed_headers: | ||
| - "*" | ||
| - "Content-Type" | ||
| - "Accept" |
There was a problem hiding this comment.
Changing allowed_origins and allowed_headers from "*" to specific values ("http://localhost:5173", "Content-Type", "Accept") is a good security practice. This follows the principle of least privilege and reduces the attack surface by preventing unauthorized cross-origin requests and header manipulation.
1 participant
feat(observability): client-side metrics via telemetry bridge
Adds a full client-side observability layer to
@moq/hang. The design keeps the library's core completely dependency-free by routing all metric calls through a thin bridge that backends can implement independently.Architecture
telemetry/bridge.ts— zero-dependency interface (TelemetryProvider) and module-level recording helpers. Events that arrive before a backend is registered are buffered and replayed onsetProvider(). High-frequency events (frames, bytes) are aggregated in Maps to prevent unbounded growth.telemetry/element.ts—<hang-telemetry>web component backed by OpenTelemetry. The SDK is dynamically imported only when a validendpointattribute is present (zero bundle cost when omitted). UsesqueueMicrotask+ a boolean flag to coalesce rapid attribute changes into a single setup cycle. CallsmeterProvider.getMeter()directly, bypassing the global singleton that would break on hot-reload reconnects.Metrics emitted
moq_client_connections_totalmoq_client_active_connectionsmoq_client_startup_time_secondsmoq_client_frames_decoded_totalmoq_client_bytes_received_totalmoq_client_bitrate_bytes_per_secondmoq_client_stall_total