Minimal observability metrics (relay & client) #853
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…port in connection file
…or better readability
WalkthroughThis pull request introduces comprehensive observability infrastructure across the MoQ codebase. The browser client now includes OpenTelemetry-based metrics collection via a new ClientMetrics class that tracks connections and startup times. The relay library adds a Stats trait and MetricsTracker for per-transport byte accounting. Both the lite and ietf protocol implementations are extended to thread an optional Stats parameter through their session layer. An optional Docker-based observability stack is configured with OTEL Collector, Prometheus, Grafana, Loki, and Tempo. Browser components are instrumented to record time-to-first-frame and time-to-first-audio events. The relay exposes a /metrics endpoint for Prometheus scraping. Documentation and configuration files for the observability setup are provided. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@observability/docker-compose.yml`:
- Around line 35-43: Update the Grafana service's ports binding to restrict
access to localhost by changing the ports entry "3050:3000" to
"127.0.0.1:3050:3000" in the docker-compose service that defines environment
variables like GF_AUTH_ANONYMOUS_ENABLED and GF_AUTH_ANONYMOUS_ORG_ROLE;
optionally change GF_AUTH_ANONYMOUS_ORG_ROLE from Admin to Viewer to reduce
anonymous privileges for local development.
In `@observability/otel-collector-config.yaml`:
- Around line 5-11: The CORS config on the OpenTelemetry collector is too
permissive: in the cors section replace wildcard settings (allowed_origins:
["*"] and allowed_headers: ["*"]) with a restricted list of known development
origins and headers (or detect local-only usage) and/or add a clear local-only
warning comment; update the cors.allowed_origins and cors.allowed_headers
entries and ensure endpoint binding (endpoint: 0.0.0.0:4318) is not exposed
publicly (bind to localhost or document network restrictions) so the collector
is only reachable from trusted developer machines or networks.
In `@observability/README.md`:
- Line 238: Replace the bare URL text "http://localhost:3050" with Markdown link
syntax or angle-bracketed URL to satisfy MD034; for example change the line that
reads 'Open http://localhost:3050 (login: admin/admin)' to 'Open
<http://localhost:3050> (login: admin/admin)' or 'Open
[http://localhost:3050](http://localhost:3050) (login: admin/admin)'. Apply the
same change to the other occurrence of the same bare URL (the line referenced
around 359) so both instances use Markdown link or angle-bracket form.
- Around line 58-60: Add a language identifier (use "text") to the fenced code
blocks that contain the ASCII diagrams so markdownlint MD040 is satisfied;
locate the blocks surrounding the diagram lines beginning with
"┌─────────────────────────────────────────────────────────────────────────────────┐"
(and similar diagrams near the "DATA SOURCES" header and the other occurrences
at the indicated sections) and change their opening triple-backtick to include
text (e.g., ```text) for each affected fenced block.
🧹 Nitpick comments (8)
js/lite/src/connection/connect.ts (1)
87-90: Guard the observability callback so it can’t break the connect flow.If the callback throws, it will currently reject
connect()even though the transport is established. Consider isolating it.♻️ Suggested change
- if (connectionTypeCallback) { - connectionTypeCallback(transportType); - } + if (connectionTypeCallback) { + try { + connectionTypeCallback(transportType); + } catch (error) { + console.warn("connection type callback failed:", error); + } + }js/hang/src/observability/index.ts (1)
57-63: Setservice.namein MeterProvider resource attributes.Currently
serviceNameis only used as the meter name, so exported metrics will not carry theservice.nameresource attribute. Add a Resource to the MeterProvider constructor to ensure observability downstream.♻️ Suggested fix
import { MeterProvider, PeriodicExportingMetricReader } from "@opentelemetry/sdk-metrics"; +import { Resource } from "@opentelemetry/resources"; const meterProvider = new MeterProvider({ readers: [reader], + resource: new Resource({ "service.name": serviceName }), });js/hang/src/watch/element.ts (2)
78-86: Hardcoded OTLP port may not be suitable for all deployments.The OTLP endpoint is derived by taking the relay's hostname and hardcoding port
4318. This assumption may break in deployments where the OTLP collector runs on a different host or port.Consider making the OTLP endpoint configurable via an attribute or environment variable, or at minimum document this coupling.
94-100: Remove empty placeholder effect.This effect block contains only a comment and no actual logic. Empty effects still have runtime cost and reduce code clarity. Remove it or implement the intended functionality.
♻️ Suggested fix
- // Create trace span for playback session after observability is initialized - this.signals.effect((effect) => { - const url = effect.get(this.url); - if (url) { - // Metrics-only observability; no tracing spans. - } - }); -rs/moq-relay/src/connection.rs (1)
16-73: Consider counting active sessions only after accept succeeds.Right now the gauge increments before auth/accept, so rejected/failed sessions can cause short-lived spikes. If you want “active_sessions” to mean established sessions, move the increment/guard to after
accept_with_statsreturns.♻️ Suggested adjustment
- let metrics = self.cluster.metrics.clone(); - metrics.inc_active_sessions(crate::Transport::WebTransport); + let metrics = self.cluster.metrics.clone(); struct SessionGuard { metrics: crate::MetricsTracker, transport: crate::Transport, @@ - let _guard = SessionGuard { - metrics: metrics.clone(), - transport: crate::Transport::WebTransport, - }; - @@ - let stats: Arc<dyn moq_lite::Stats> = - Arc::new(crate::TransportStats::new(metrics, crate::Transport::WebTransport)); - let session = self.request.accept_with_stats(subscribe, publish, Some(stats)).await?; + let stats: Arc<dyn moq_lite::Stats> = + Arc::new(crate::TransportStats::new(metrics.clone(), crate::Transport::WebTransport)); + let session = self.request.accept_with_stats(subscribe, publish, Some(stats)).await?; + metrics.inc_active_sessions(crate::Transport::WebTransport); + let _guard = SessionGuard { + metrics, + transport: crate::Transport::WebTransport, + };observability/docker-compose.yml (1)
2-35: Pin image versions for reproducible demo runs.Using
:latestfor otel-collector/Prometheus/Grafana can change behavior over time. Consider explicit tags or digests (possibly via a.envfile) to keep the demo stable.rs/moq-relay/src/web.rs (1)
170-216: Consider setting theContent-Typeheader for Prometheus compatibility.The Prometheus text exposition format specifies
text/plain; version=0.0.4; charset=utf-8as the Content-Type. While many scrapers are lenient, setting the correct header ensures full compatibility.Suggested fix
-async fn serve_metrics(State(state): State<Arc<WebState>>) -> String { +async fn serve_metrics(State(state): State<Arc<WebState>>) -> impl IntoResponse { // Minimal Prometheus text exposition. let m = &state.cluster.metrics; let wt = crate::Transport::WebTransport; let ws = crate::Transport::WebSocket; let mut out = String::new(); // ... rest of the function ... - out + ( + [("content-type", "text/plain; version=0.0.4; charset=utf-8")], + out, + ) }rs/moq-relay/src/metrics.rs (1)
26-54: Consider using saturating subtraction fordec_active_sessions.While underflow here would indicate a programming bug, using
fetch_updatewithsaturating_subcould prevent silent wraparound in case of mismatched inc/dec calls during development.Optional defensive change
pub fn dec_active_sessions(&self, transport: Transport) { match transport { - Transport::WebTransport => self.active_sessions_webtransport.fetch_sub(1, Ordering::Relaxed), - Transport::WebSocket => self.active_sessions_websocket.fetch_sub(1, Ordering::Relaxed), + Transport::WebTransport => { + self.active_sessions_webtransport.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |v| Some(v.saturating_sub(1))).ok(); + } + Transport::WebSocket => { + self.active_sessions_websocket.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |v| Some(v.saturating_sub(1))).ok(); + } }; }
| environment: | ||
| - GF_AUTH_ANONYMOUS_ENABLED=true | ||
| - GF_AUTH_ANONYMOUS_ORG_ROLE=Admin | ||
| - GF_AUTH_DISABLE_LOGIN_FORM=false | ||
| volumes: | ||
| - grafana-data:/var/lib/grafana | ||
| - ./grafana/provisioning:/etc/grafana/provisioning:ro | ||
| ports: | ||
| - "3050:3000" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "docker-compose.yml" -type f | head -20Repository: moq-dev/moq
Length of output: 89
🏁 Script executed:
cat -n observability/docker-compose.ymlRepository: moq-dev/moq
Length of output: 1919
🏁 Script executed:
# Verify Docker Compose port binding behavior documentation
cat << 'EOF' > /tmp/verify_docker.txt
Port binding syntax in docker-compose:
"3050:3000" without explicit IP means bind to 0.0.0.0:3050
"127.0.0.1:3050:3000" explicitly binds to localhost only
Current config: "3050:3000" → binds to all interfaces (0.0.0.0)
EOF
cat /tmp/verify_docker.txtRepository: moq-dev/moq
Length of output: 274
🏁 Script executed:
# Check if there are any ingress controllers, firewalls, or network restrictions
# that might mitigate the exposure risk
rg -i "ingress|firewall|securitypolicy|networkpolicy" --type yaml --type yml .Repository: moq-dev/moq
Length of output: 82
🏁 Script executed:
# Check for documentation about the observability stack
find . -path ./observability -prune -o -name "README*" -o -name "*.md" | head -20
cat -n observability/*.md 2>/dev/null || echo "No markdown in observability dir"Repository: moq-dev/moq
Length of output: 23356
🏁 Script executed:
# Check if there's a main README or setup docs
head -50 README.md 2>/dev/null | grep -i "observability\|development\|docker" || echo "Checking root context..."Repository: moq-dev/moq
Length of output: 273
For a local development stack, consider binding Grafana to localhost for safer defaults.
This is optional development tooling, but restricting Grafana to localhost even in dev avoids accidental exposure. Bind to 127.0.0.1:3050:3000 instead of 3050:3000, and optionally downgrade anonymous role from Admin to Viewer if needed for testing.
Suggested change
grafana:
image: grafana/grafana:latest
environment:
- GF_AUTH_ANONYMOUS_ENABLED=true
- GF_AUTH_ANONYMOUS_ORG_ROLE=Admin
- GF_AUTH_DISABLE_LOGIN_FORM=false
volumes:
- grafana-data:/var/lib/grafana
- ./grafana/provisioning:/etc/grafana/provisioning:ro
ports:
- - "3050:3000"
+ - "127.0.0.1:3050:3000"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| environment: | |
| - GF_AUTH_ANONYMOUS_ENABLED=true | |
| - GF_AUTH_ANONYMOUS_ORG_ROLE=Admin | |
| - GF_AUTH_DISABLE_LOGIN_FORM=false | |
| volumes: | |
| - grafana-data:/var/lib/grafana | |
| - ./grafana/provisioning:/etc/grafana/provisioning:ro | |
| ports: | |
| - "3050:3000" | |
| environment: | |
| - GF_AUTH_ANONYMOUS_ENABLED=true | |
| - GF_AUTH_ANONYMOUS_ORG_ROLE=Admin | |
| - GF_AUTH_DISABLE_LOGIN_FORM=false | |
| volumes: | |
| - grafana-data:/var/lib/grafana | |
| - ./grafana/provisioning:/etc/grafana/provisioning:ro | |
| ports: | |
| - "127.0.0.1:3050:3000" |
🤖 Prompt for AI Agents
In `@observability/docker-compose.yml` around lines 35 - 43, Update the Grafana
service's ports binding to restrict access to localhost by changing the ports
entry "3050:3000" to "127.0.0.1:3050:3000" in the docker-compose service that
defines environment variables like GF_AUTH_ANONYMOUS_ENABLED and
GF_AUTH_ANONYMOUS_ORG_ROLE; optionally change GF_AUTH_ANONYMOUS_ORG_ROLE from
Admin to Viewer to reduce anonymous privileges for local development.
| endpoint: 0.0.0.0:4318 | ||
| cors: | ||
| allowed_origins: | ||
| - "*" | ||
| allowed_headers: | ||
| - "*" | ||
| max_age: 7200 |
There was a problem hiding this comment.
Wildcard CORS on the collector is risky outside local demos.
allowed_origins: ["*"] and allowed_headers: ["*"] let any origin POST metrics if the collector is reachable. Please restrict CORS to known dev origins or add an explicit “local-only” warning and ensure the stack isn’t exposed beyond the developer machine/network.
🤖 Prompt for AI Agents
In `@observability/otel-collector-config.yaml` around lines 5 - 11, The CORS
config on the OpenTelemetry collector is too permissive: in the cors section
replace wildcard settings (allowed_origins: ["*"] and allowed_headers: ["*"])
with a restricted list of known development origins and headers (or detect
local-only usage) and/or add a clear local-only warning comment; update the
cors.allowed_origins and cors.allowed_headers entries and ensure endpoint
binding (endpoint: 0.0.0.0:4318) is not exposed publicly (bind to localhost or
document network restrictions) so the collector is only reachable from trusted
developer machines or networks.
| ``` | ||
| ┌─────────────────────────────────────────────────────────────────────────────────┐ | ||
| │ DATA SOURCES │ |
There was a problem hiding this comment.
Add language identifiers to fenced blocks to satisfy markdownlint (MD040).
These fenced blocks lack a language tag. Consider using text for ASCII diagrams/flows.
✅ Example fix (apply similarly to the other blocks)
-```
+```text
┌─────────────────────────────────────────────────────────────────────────────────┐Also applies to: 136-138, 167-169, 197-199
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
58-58: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@observability/README.md` around lines 58 - 60, Add a language identifier (use
"text") to the fenced code blocks that contain the ASCII diagrams so
markdownlint MD040 is satisfied; locate the blocks surrounding the diagram lines
beginning with
"┌─────────────────────────────────────────────────────────────────────────────────┐"
(and similar diagrams near the "DATA SOURCES" header and the other occurrences
at the indicated sections) and change their opening triple-backtick to include
text (e.g., ```text) for each affected fenced block.
|
|
||
| ### 4. Access Grafana | ||
|
|
||
| Open http://localhost:3050 (login: admin/admin) |
There was a problem hiding this comment.
Replace bare URLs with Markdown links to satisfy markdownlint (MD034).
Wrap the bare URLs in link syntax (or angle brackets) to pass lint.
✅ Example fix
-Open http://localhost:3050 (login: admin/admin)
+Open <http://localhost:3050> (login: admin/admin)Also applies to: 359-359
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
238-238: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In `@observability/README.md` at line 238, Replace the bare URL text
"http://localhost:3050" with Markdown link syntax or angle-bracketed URL to
satisfy MD034; for example change the line that reads 'Open
http://localhost:3050 (login: admin/admin)' to 'Open <http://localhost:3050>
(login: admin/admin)' or 'Open [http://localhost:3050](http://localhost:3050)
(login: admin/admin)'. Apply the same change to the other occurrence of the same
bare URL (the line referenced around 359) so both instances use Markdown link or
angle-bracket form.
1 participant
Summary
This PR adds the smallest reviewable observability surface for the MoQ demo: a reusable application-level byte accounting hook in
moq-lite, minimal relay-side counters exposed via a Prometheus-style/metricsendpoint, and minimal browser-side OTLP metrics for connection type + time-to-first-frame.What’s included
moq-lite: optionalStatssink for application payload byte accountingStatstrait andconnect_with_stats/accept_with_statsplumbingmoq-native: addsRequest::accept_with_stats(...)to pass the sink through tomoq-litemoq-relay: minimal counters +/metricsmoq_relay_active_sessions{transport="webtransport|websocket"}(gauge)moq_relay_app_bytes_sent_total{transport=...}(counter)moq_relay_app_bytes_received_total{transport=...}(counter)js/hang: metrics-only OTLP HTTP export (no tracing)moq_client_connections_total{transport=...}(counter)moq_client_startup_time_seconds{track_type="audio|video"}(histogram, time-to-first-frame from track subscription start)js/lite: adds a tinyonConnectionType(cb)callback hook so hang can recordwebtransportvswebsocketjs/hangvideo stats:framesDecodedis counted on the decoder output path; removed the unimplementedframesDroppedWhat’s intentionally out of scope
How to test (local)
You should see
moq_relay_app_bytes_*increasing while playback is active.:4318, the browser will POST metrics to:http://<collector>:4318/v1/metricsNotes for reviewers
moq-lite::Statshook is intentionally generic and non-blocking (callers can implement with atomics)./metricsuses low-cardinality labels only (transport), matching Prometheus best practices.