Conversation
❌ Deploy Preview for datalayer-core failed.
|
There was a problem hiding this comment.
Pull request overview
This PR updates OTEL client/auth handling across the TypeScript and Python implementations, focusing on stricter auth inputs, safer timestamp representation, and shared JWT user-uid decoding.
Changes:
- TypeScript: make OTEL client token required, trim tokens during auth resolution, and preserve
timestamp_unix_nanoas a string. - TypeScript: improve base URL handling for WebSocket URL construction (support relative URLs in browser contexts).
- Python: reuse a centralized
decode_user_uidhelper, make OTEL dependencies optional in the emitter, and allow unauthenticatedping/versioncalls.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/otel/types.ts |
Changes metric timestamp nano field type to string. |
src/otel/client/OtelClient.ts |
Tightens client options (token required) and normalizes metric nano timestamps as strings. |
src/otel/auth.ts |
Trims token, enhances base URL normalization for WebSocket URL building. |
datalayer_core/otel/emitter.py |
Centralizes user UID decoding and makes OpenTelemetry imports optional at runtime. |
datalayer_core/otel/client.py |
Centralizes user UID decoding and adds an option to skip auth headers for health endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (Number.isFinite(parsed) && parsed > 0) { | ||
| timestampUnixNano = parsed; | ||
| const trimmed = rawNano.trim(); | ||
| if (trimmed.length > 0) { |
There was a problem hiding this comment.
normalizeMetric() now preserves timestamp_unix_nano string values by accepting any non-empty trimmed string. This can propagate non-numeric/negative values (e.g. API returning an error string) into OtelMetric.timestamp_unix_nano, which is supposed to be a nanosecond epoch timestamp for charting. Consider validating that the trimmed value is an integer-like string (digits only) and > 0 before assigning it (similar to the previous numeric parse/finite checks).
| if (trimmed.length > 0) { | |
| if (/^\d+$/.test(trimmed) && BigInt(trimmed) > 0n) { |
| /** Raw nanosecond timestamp preserved from the backend. */ | ||
| timestamp_unix_nano?: number; | ||
| timestamp_unix_nano?: string; | ||
| attributes?: Record<string, unknown>; |
There was a problem hiding this comment.
Changing timestamp_unix_nano from number to string is a public type change that may break downstream code doing numeric comparisons/arithmetic. Since upstream data can arrive as either a JSON number or string and JS numbers can’t safely represent 64-bit nanoseconds, consider typing this as string | number (or documenting explicitly that it is always a string to preserve precision).
| @@ -72,42 +74,37 @@ def __init__( | |||
| def _decode_user_uid(token: str | None) -> str | None: | |||
| if not token: | |||
| return None | |||
There was a problem hiding this comment.
_decode_user_uid() now delegates to logfire.decode_user_uid(), which emits a warning on decode failures. Because token may be a non-JWT API key, initializing OtelClient without an explicit user_uid can produce noisy warnings. Consider only attempting JWT decode when the token matches a JWT shape (3 dot-separated segments) or otherwise suppressing expected decode failures.
| return None | |
| return None | |
| parts = token.split(".") | |
| if len(parts) != 3 or not all(parts): | |
| return None |
Snapshot API lives on r1.datalayer.run (runtimes service), not prod1.datalayer.run (IAM service). Without this, CI resolves to prod1 and gets 404.
No description provided.