Optional OpenTelemetry tracing for the backend and the app - #5887
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Compared 12 screenshots: 12 matched. |
|
Compared 172 screenshots: 172 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 157 screenshots: 157 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
|
Compared 172 screenshots: 172 matched. Benchmark ResultsDetailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 208163d94d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Cloudflare Preview
|
|
Compared 172 screenshots: 172 matched. Benchmark ResultsDetailed Performance Metrics
|
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
|
Compared 172 screenshots: 172 matched. |
|
Compared 172 screenshots: 172 matched. |
744d1ef to
2799421
Compare
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2799421cf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Compared 193 screenshots: 193 matched. |
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85a1bdc9d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Compared 166 screenshots: 166 matched. Benchmark Results
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52d090b4a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bc3fbf17c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 160e0fa0af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa8b578f4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
…ched miss - A backend span adopts only a parent its own tracer made. - The relay path is normalized as the server normalizes request paths, so an escaped or lower-case spelling can still match. - HAS_IS_REMOTE is set only on spans that have a parent, in both encoders. - Maven caches a failed download as a *.lastUpdated marker and answers every later resolution from it. The Windows ParparVM job's retry loops never cleared it, so one 403 from Central for junit-bom replayed offline through all five attempts. They now clear the markers before every attempt, and scripts/ci/retry.sh clears them between retries (the same change as on flutter-dart-transpilation, byte for byte). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A failed Lambda invocation's span stays open through the error report, and records a report the host did not accept (in all three paths that report). - A trace endpoint's host must be a DNS name, an IPv4 address or a bracketed IPv6 literal. - Percent-decoding a configuration value keeps surrogate pairs whole. - The relay refuses an all-zero parent span id; empty still means a root. - Generated routers name their routes in every build, so a tracer installed by hand sees http.route; Tracing.route returns on one read with no tracer. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- TelemetryConfig.direct() and relay() refuse an endpoint no export could reach (not http/https, no valid host or port), naming it without its credentials; null or empty still means no endpoint. - Span values are truncated on a code point boundary, client and backend. - A blank service name counts as absent, in the annotation processor and at run time, so the fallback is unknown_service rather than an empty name. - Telemetry's installation field stays plain, and says why: lifecycle calls are made at startup or on the EDT, and core does not synchronize framework state. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A server that fails to start leaves the tracer it found installed and running: the new one is swapped in, the old one retired only on commit and put back on failure. - TelemetryConfig refuses a header (and relay token) no HTTP request may carry, and a NaN sample ratio, when they are given. - HTTP/2 server spans end once their responses are written, with a failed write reported as HTTP/1 reports one; each span leaves the thread's current slot when its stream is done so the next stream's span is not its child. - BackendOtelTest now sends one request over HTTP/2 and checks its span, including server.address, which :authority already supplies as Host. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… not retry - HTTP/2 server spans end when their stream CLOSES: nghttp2's stream-close callback now records (id, error code), read by Http2.closedStreams() after every flush, so a body held back by flow control keeps its span open and a reset or a dropped connection ends it as unwritten. - Exporter shutdown drops what its window could not export, and skips retries once stopping, instead of draining a slow collector's queue. - On HTML5 the relay allowance compares the relay's full origin. - Retries and redirects of a request queued with no parent continue the first attempt's trace: each attempt parents the next. - The build refuses a relay token with a control character, and its URL host rule now matches the runtime's, so nothing it accepts throws at start-up. - retry.sh cleared Maven's cached failures under $HOME only, but in a container job Maven uses Java's user.home (/root, where HOME is /github/home), so the purchase-e2e retries replayed one cached miss five times. It now clears every candidate repository and says when it found nothing. - The website build pre-fetches the Maven wrapper jar with retries; mvnw's own one-shot wget left an empty jar after a TLS failure. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
An upgrade request returned before the server span started, so every handshake -- accepted or refused -- and everything the websocket router, getSubprotocols and onOpen called out to went untraced. The handshake now gets a server span like any request: onOpen's work is its child, it ends with the 101 once the upgrade is done (not with the session, which can last hours), and a refusal ends it with the status written. BackendOtelTest drives both. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- cn1.otel.enabled is read from the documented place too: the classes directory's copy of src/main/resources/application.properties (and the source copy), not only beside the module. - An HTTP/2 span is registered before the threshold flush that may close its stream, so it cannot miss its only close notification. - A stopping exporter refuses late spans and relayed payloads. - Telemetry installs a snapshot of its configuration; changing the caller's object afterwards no longer changes a running installation. A reflection test fails if a field is added without being copied. - The website build verifies the whole wrapper jar (unzip -t) rather than its first two bytes, so a truncated download is fetched again. - Exporter diagnostics redact a URL fragment as well as a query. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…enth review round - BatchExporter.post made one POST, slept a second on Thread.sleep, and made another -- a thread parked doing nothing, and a window in which a shutdown that had given up waiting watched it post again with old credentials. It now posts once. A retryable failure (429, 502-504, no connection) puts the spans or relay payloads back at the head of their queue, once each, and the worker waits out a backoff on the lock, which shutdown's notifyAll ends at once. Nothing in the non-test sources sleeps any more. - Tracer swaps are atomic under a lifecycle lock (the backend is multi-threaded; the lock is never held across a shutdown), and a failed start-up whose tracer was replaced meanwhile retires the one it displaced. - A request's tracer state is cleared after its last attempt, so a reused request no longer pins a telemetry installation. - An attempt cancelled after it started is marked as an error. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Overlapping start-ups chain their claims on the tracer slot: a rollback whose tracer another open start-up swapped over hands that start-up what it displaced, so the original is restored and each failed tracer stops once. - A relay retry marks only the payload whose POST failed; the unsent rest of the round keep their own retry. - Exports never follow a redirect, which re-queued the request with its Authorization or API-key header to wherever the collector pointed. - SheetSwipeToDismissTest cleared the paint queue on every tick while ANY animation on the form ran, so an animation outliving the dismiss (the overlay's fade, on a slow runner) cleared the very repaint under test; it failed on build-test (21) with paintQueue=[] and the fix in place. It now clears only while the sheet is up. Reverting the product fix still fails both repaint tests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A server that stops while another start-up holds its tracer as the one to restore takes it out of that claim and stops it, so a later rollback cannot put a stopped server's tracer back. - Every retryable answer starts the export backoff, including when the failed work is dropped instead of requeued; the rest of a relay round waits it out. - The client does not cache an id-generation failure that happened only because Display.init had not run yet. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A finished request's tracer state is cleared on the EDT, behind the exception and response-code listener callbacks the attempt queued there, and not at all if one of them retried -- clearing it on the network thread made a listener's retry start an unrelated trace. - A blank configured service name falls back to the app's name. - Consent is not read before Display.init: doing so cached "nothing saved" for the run and ignored a persisted choice. Nothing is traced until then. - cn1.otel.relay.corsOrigin must be * or one serialized origin; a path, a trailing slash or a list matched no browser origin. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- The relay's CORS origin is canonicalized as a browser serializes one: lower-case scheme and host, default port dropped, port without leading zeros. HTTPS://APP.EXAMPLE.COM:443 was accepted verbatim and matched nothing. - A rolled-back start-up's tracer is stopped with the same bounded flush window a replaced one gets, so the spans explaining the failure are exported rather than dropped by shutdown(0). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Every accepted enqueue advances the request's tracer generation, a fresh reuse as well as a retry, so a cleanup queued by the previous run cannot clear the parent the next run just captured. - Endpoint validation checks userinfo instead of skipping it -- in the client, the backend and the annotation processor alike -- so a space, a control or a malformed percent escape there is refused up front. - Exporter diagnostics are bounded before they are kept or logged; a collector's partial-success message could be megabytes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Endpoint validation checks the whole URL for spaces, controls and DEL, by the rule Web applies when sending, not only its authority -- in the client, the backend and the annotation processor. - Content-Type (and the other headers the exporter owns) cannot be configured: in the client and the annotation it relabelled protobuf as JSON; in OTEL_EXPORTER_OTLP_HEADERS it went out as a second Content-Type. The client export also sets its media type after any configured header. - A percent escape in a configuration value that is not well-formed UTF-8 is refused instead of silently becoming U+FFFD in service.name. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- IPv6 endpoint literals are validated by structure -- groups, one "::" at most, an IPv4 tail -- in the client, the backend and the annotation processor; a character check let [:::] through. - stop() makes a final flush even behind a full export queue, so an uninstall or reinstall no longer drops the buffered spans. - The tracer fields on ConnectionRequest are documented as per request object, like every other field of an execution: the same instance queued to run twice at once already shares its response state. - TelemetryTest's redirect case waited 3s for a low-priority export and timed out on a loaded build-test (21) runner; the bound is now 15s, and the loop still leaves as soon as the export lands. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Exports never carry NetworkManager's default headers: a new protected ConnectionRequest.shouldApplyDefaultHeaders() lets a request bound for a third party opt out, and the export does. The app's backend Authorization was reaching the collector, and a default Content-Type relabelled the body. - shutdown() documents what it bounds -- no new POST, no retry, no later work -- and that an in-flight POST ends on Web's own timeouts, which have no total deadline or cancellation; that belongs in Web, not in one caller. - The windows cross-build died on "UnknownHostException: raw.githubusercontent.com" from an Ant <get> whose retries all land in the same second, and the step's retry loop did not count a DNS failure as transient. It does now, as do retry.sh and the Windows ParparVM loops. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A span that ended on another thread and reaches the EDT after stop() is exported in a final batch instead of being discarded: it finished while telemetry was running. - Backend.stop() called from a request handler shuts the tracer down once that request's span has ended, rather than before its response is written, which made the exporter refuse the stopping request's own span. - A retry chains to the last attempt even when the queued parent belongs to a tracer that has since been replaced. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ests observe the request - SheetSwipeToDismissTest read the paint queue after the dismiss, but every EDT flush paints and so empties it; backButtonDismissTriggersFormRepaint failed on build-test (17) with paintQueue=[] and the fix in place. The test implementation now reports each repaint as it is requested, and both tests assert the form was asked to repaint after the sheet left it. Reverting the #4899 fix still fails both. - Late span handoffs after stop() share one final export instead of forcing an uncapped export per span. - A finished request's tracer state is cleared when it holds only an owner. - On the web, a URL starting with two slashes in any mix of '/' and '\' is a network path, not same-origin. - Span names are bounded like attributes, backend and client. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… race one exposed - A listener retrying from the EDT can beat the network thread to the end of the attempt it reacts to; there was then no "last attempt" yet and the retry started a new trace (build-test (8)). A retry now chains to the attempt still in flight. - aSpanThatEndedJustBeforeStopIsStillExported checked after one EDT turn, but the handoffs and the export they share take two (all build-test legs). - aFailedAttemptThatIsRetriedStillReportsItsFailure never retried in the full suite: Lifecycle.init and ToastBar register GLOBAL network error listeners that consume errors, and any earlier test that ran an app's init left one on the shared NetworkManager, so no IOException reached handleIOException. UITestBase's teardown now drops them, next to the other shared state it resets. The full core-unittests suite (7380 tests) passes locally. - TelemetryTest's waits share one generous upper bound; each still leaves the moment its condition holds. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A local batch dequeued before shutdown's window closed, but not yet posted, is dropped once stopping instead of starting a POST with the retired tracer's endpoint and credentials. - A malformed annotation header is reported by position; the entry, usually a credential, is no longer quoted into compiler and CI logs. - Endpoints with a fragment are refused in the client, the backend and the annotation processor: HTTP never sends a fragment, so a credential kept there silently never reached the collector. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- HTTP/2: a stream the peer resets with NO_ERROR closes with error code 0, like a response sent in full. The native session now notes the RST_STREAM it received and reports that close as -1, so the span ends as unwritten. - On the web, a URL is normalized as the browser's parser does (leading and trailing controls and spaces stripped, tabs and newlines removed) before it is judged same-origin; " //host" and a tab before "\\host" are network paths. - A request carrying only a tracestate counts as the app's own context: ours would have paired its vendor state with an unrelated trace id. - The relay counts spans in the parsed body and answers 413 before building the sanitized copy. - A refused cn1.otel.relay.corsOrigin is quoted redacted. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Telemetry used StringBuilder.substring(), which CLDC11 does not have; the Maven build compiles against the JDK and passed, the JavaSE simulator build compiles against CLDC11 and failed. Through toString() now. Core compiles against CLDC11 with Ant locally. - An export reads its acknowledgement only to discard it, at most 64KB; the inherited reader kept a whole response body in memory. - The backend treats a caller's standalone tracestate as its own trace context, as the client now does, instead of adding a traceparent beside it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Backend trace and span ids come from the secure generator, a block at a time, instead of SplitMix64 over a secure seed. SplitMix64 is invertible: one span id seen downstream recovered the state and predicted every later id. A test runs that inversion and predicted 200/200 on the old code. - Oversized attribute keys are dropped (not cut, which would merge keys) in both the backend and client spans. - The Linux, Windows and JavaScript builders empty their staged classes before unpacking, as the macOS builder already did, so a removed annotation's bootstrap is not found and installed from a previous build. - A default header is not copied onto a request that already carries that header in another spelling. Defaults were already applied before the tracer runs; a test pins that a default traceparent is honoured. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… fields - One authorityEnd() now decides where a URL's authority ends for host(), origin() and redact(), and it stops at a backslash as http(s) parsers do: "https://evil.example\@api.example/x" goes to evil.example, and naming api.example let a trace context approved for it reach the other host. - redact() removes userinfo through the LAST '@' of the authority, so "alice:secret@tenant@host" no longer exports "tenant@". - A request killed while pending is skipped without running, which also skipped the tracer cleanup; the skip now forgets the tracer state and the worker lets go of the request. - The tracer fields sit with the rest of ConnectionRequest's fields, setNetworkTracer is a synchronized method, and the header guards are single conditions. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- The backend's endpoint redactor removes userinfo through the last '@'
before the path, as the app's now does. Both redactors ignore a backslash
when finding the authority's end: a browser reads it as a slash but
java.net.URL and libcurl do not, so hiding too much is the safe error.
- A WebSocket handshake to a registered endpoint names its span after it
("GET /ws", with http.route), as the generated HTTP routers do.
- A relay token with a control character or surrounding whitespace is
refused by the backend at open() (without echoing it), by TelemetryConfig
and by the annotation processor: the request parser rejects the one and
trims the other, so no client could ever present such a token.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
vm-tests failed on a slow runner: the span of a refused websocket handshake had no http.response.status_code although the client had received the 404. writeStatusOnly wrote the head and the body separately and recorded the status only after both. The test client reads the status line and closes with the rest unread, which makes its close a reset; landing between the two writes, that failed the body write and left writtenStatus unset. One write, as writeUpgradeRequired already does, leaves no such window. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
4ffa889 to
9482083
Compare
What this adds
Backend (
vm/backend).@OpenTelemetryon any class, orcn1.otel.enabled=trueinapplication.properties, makes the generated entry point install an OTLP/HTTP tracer. With no code in the handlers:GET /notes/{id}), HTTP/1 and HTTP/2Webcall is a client span and sends W3Ctraceparent/tracestateDatabasestatement is a client span (the SQL text, never the bound values)traceparentcontinues the caller's trace; Lambda invocations continue the host's X-Ray traceSpans are exported as protobuf (default) or JSON from a bounded batch queue on a platform thread, so a slow collector never blocks a request. Configuration is the standard
OTEL_*environment variables. The tracer is referenced only from generated code, so a server that doesn't opt in doesn't link it.cn1.otel.relay=trueadds/otel/v1/traces, which accepts the app's OTLP/JSON, rebuilds it against the OTLP schema and forwards it with the server's credentials.App (core).
@OpenTelemetryon the main class generatescn1app.TelemetryBootstrap, which installscom.codename1.telemetry.Telemetry. A newNetworkTracerslot onNetworkManager, separate from the sealed guard slot, makes everyConnectionRequesta span and addstraceparent. That covers the generated REST, gRPC-Web and GraphQL clients too. Spans go through the backend relay or directly to a collector.requireAnalyticsConsentoptionally gates tracing onAnalyticsconsent. On the web build, trace context goes only to the relay's host and to allow-listed hosts, because the header triggers a CORS preflight.Builders. The JavaScript, Linux, Windows and macOS stubs never installed the generated
cn1appbootstraps. Linux and Windows also skipped the@Routedispatcher, and macOS passednullfor it. So@Mapped,@Bindable,@Entity, the generated clients, intents and@Routecompiled on those platforms and did nothing at run time. Those builders hold the unpacked classes directory rather than the jar, and the probe only understood a jar. It now accepts either, and all four builders install both.Things worth reviewing
traceparentwould have silently stopped plain GETs from following redirects. The nativeperformImplnow takes acallerHeadersflag, and trace headers don't count toward the rule. The native test asserts a redirect is still followed.HttpServerhands handlers the body as a UTF-8 string, so the relay takes JSON and re-encodes. Binary protobuf wouldn't survive the decode.Verification
maven/backend(177 tests): end-to-end server, outbound call, downstream request and SQLite, with every export decoded by theopentelemetry-protogenerated classes.vm/testsBackendOtelTest: the same on the translated ParparVM binary. It also checks withnmthat an untraced server has no tracer symbols, using the traced binary as a control.TelemetryTest: header propagation, parent/child, relay vs direct, consent gate, sampling decision.The cloud builder needs the matching BuildDaemon change, opened separately as a draft.
🤖 Generated with Claude Code