Repository navigation
Conversation
github-actions
Bot
added
dependencies
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5376 +/- ##
============================================
+ Coverage 92.33% 92.36% +0.02%
+ Complexity 5046 5045 -1
============================================
Files 1261 1261
Lines 54118 54077 -41
Branches 6767 6764 -3
============================================
- Hits 49971 49946 -25
+ Misses 2487 2475 -12
+ Partials 1660 1656 -4
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 667 | 0.407 | 14,797/20,501/20,501 us | 🔴 +30.0% / 🔴 +45.2% |
| 🔴 | bs=100 sw=10 sl=64 | 1,651 | 1.008 | 58,387/85,752/85,752 us | 🔴 +22.5% / 🟢 +31.9% |
| 🟢 | bs=1000 sw=10 sl=64 | 1,998 | 1.219 | 499,568/556,262/556,262 us | 🟢 -6.4% / 🟢 +51.3% |
Baseline details
Latest main c7b4880 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 667 tuples/sec | 834 tuples/sec | 968.33 tuples/sec | -20.0% | -31.1% |
| bs=10 sw=10 sl=64 | MB/s | 0.407 MB/s | 0.509 MB/s | 0.591 MB/s | -20.0% | -31.1% |
| bs=10 sw=10 sl=64 | p50 | 14,797 us | 11,385 us | 10,646 us | +30.0% | +39.0% |
| bs=10 sw=10 sl=64 | p95 | 20,501 us | 20,010 us | 14,120 us | +2.5% | +45.2% |
| bs=10 sw=10 sl=64 | p99 | 20,501 us | 20,010 us | 16,498 us | +2.5% | +24.3% |
| bs=100 sw=10 sl=64 | throughput | 1,651 tuples/sec | 1,706 tuples/sec | 1,252 tuples/sec | -3.2% | +31.9% |
| bs=100 sw=10 sl=64 | MB/s | 1.008 MB/s | 1.041 MB/s | 0.764 MB/s | -3.2% | +31.9% |
| bs=100 sw=10 sl=64 | p50 | 58,387 us | 57,940 us | 83,736 us | +0.8% | -30.3% |
| bs=100 sw=10 sl=64 | p95 | 85,752 us | 70,000 us | 92,464 us | +22.5% | -7.3% |
| bs=100 sw=10 sl=64 | p99 | 85,752 us | 70,000 us | 108,561 us | +22.5% | -21.0% |
| bs=1000 sw=10 sl=64 | throughput | 1,998 tuples/sec | 1,988 tuples/sec | 1,321 tuples/sec | +0.5% | +51.3% |
| bs=1000 sw=10 sl=64 | MB/s | 1.219 MB/s | 1.213 MB/s | 0.806 MB/s | +0.5% | +51.2% |
| bs=1000 sw=10 sl=64 | p50 | 499,568 us | 500,355 us | 811,260 us | -0.2% | -38.4% |
| bs=1000 sw=10 sl=64 | p95 | 556,262 us | 594,330 us | 847,636 us | -6.4% | -34.4% |
| bs=1000 sw=10 sl=64 | p99 | 556,262 us | 594,330 us | 879,277 us | -6.4% | -36.7% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,299.94,200,128000,667,0.407,14796.73,20501.28,20501.28
1,100,10,64,20,1211.59,2000,1280000,1651,1.008,58386.71,85752.00,85752.00
2,1000,10,64,20,10012.42,20000,12800000,1998,1.219,499568.31,556262.07,556262.07
Automated Reviewer SuggestionsBased on the
|
|
I left some comments in this PR, specifically in one of these comments I put a link to otel official demo app written in Java and we should follow official guidelines m. |
…WorkflowMetrics rename Follow the OTel Java demo patterns instead of custom wrappers (zuozhiw review): - WorkflowService: start the run-level span with the standard OTel API, name it WorkflowService.initExecutionService, and drop the initExecutionServiceSpanned split so no span is passed as an argument. The real execution failure is now recorded onto the span from errorHandler, where it is actually caught. - TexeraTracer: drop the withSpan wrapper, keeping only the tracer accessor (single instrumentation scope) and currentContext. - SpanAttrs: drop the awkward with*/set* setter helpers, keep the standard label keys and sanitizeFreeText; callers set attributes via the OTel API. - Rename TexeraMetrics to WorkflowMetrics, document that this facade is only for the workflow-execution cluster, and add an example of calling the OTel meter API directly at a call site. Update SpanAttrsSpec and WorkflowMetricsSpec to match.
zuozhiw
left a comment
zuozhiw
left a comment
There was a problem hiding this comment.
changes look good overall, left a few more comments, after fixing them you can go ahead and merge
| ): Unit = { | ||
| val span = TexeraTracer.tracer | ||
| .spanBuilder("WorkflowService.initExecutionService") | ||
| .setAttribute("texera.workflow.id", workflowId.id.toString) |
There was a problem hiding this comment.
minor: use standard keys instead of plain string
There was a problem hiding this comment.
Switched the attribute keys from plain strings to the shared typed keys (texera.workflow.id and texera.execution.id).
| state.withExecutionId(workflowContext.executionId) | ||
| ) | ||
| val errorHandler: Throwable => Unit = { t => | ||
| { |
There was a problem hiding this comment.
this error handler needs to be extra careful becaue it's passed around as an argument to WorkflowExecutionService, and it will be called beyond this function's lifecycle, we really need to check who calls it. when they call it this specific span object we get here might already ended after this function ends. this handler is also used locally, that is fine, but if other places call it then we have troubles and the error is not attached to the correct span
this one is a bit tricky, as we need to inspect the places that invokes this error handler and see what spans they have, then we either call span.current() again inside this span (but need to make sure the callers has good spans...), or has to pass in a span as optinal argument, please check how we are using this handler and see what's the best way to handle this, maybe passing such error handler around is not a good error handling method
There was a problem hiding this comment.
Kept the shared error handler from writing to the span, since it can run later after the span has already closed. Setup failures that happen right away are now recorded on the span by initExecutionService while it is still open.
|
This pull request has been automatically marked as stale because it has If this PR is still relevant, please:
To exempt this PR from auto-close, a maintainer can add the |
b372b82 to
38d493e
Compare
38d493e to
101b7f2
Compare
|
@parshimers @sshiv012 @zuozhiw re-pinging for review. Scope is unchanged here (backend metrics and tracing emission primitives); this is PR 2 of the kept three-PR instrument-and-export stack (#5375 to #5377). Could you re-review when you get a chance? Thanks. |
There was a problem hiding this comment.
Please fix the setup-error recording below: a compilation failure can reach the UI without being recorded as an error in the setup span. Add focused tests for a synchronous compilation failure and a callback arriving after setup has ended.
For follow-up tracing work, I need connected coverage across REST, WebSocket commands, the execution engine, Java/Scala workers, and Python workers. For example, when a Python operator fails after a user starts a workflow over WebSocket, we should be able to follow that command through the engine to the failing worker in one trace. A setup-only span cannot explain that failure or where the execution spent its time.
This can span multiple PRs. Please track a follow-up covering:
- Spans for incoming/outgoing HTTP requests and meaningful WebSocket commands.
- A workflow execution span lasting until completion, failure, or cancellation, with child spans for setup and worker operations.
- Standard W3C trace context (trace and parent-span IDs) carried through engine RPC/messages and the JVM/Python transport, and restored around asynchronous work.
AsyncRPCClient/AsyncRPCServer and the Python proxy are useful central places to implement propagation. Add workflow/execution/operator/worker IDs to the relevant spans and logs. An eventual acceptance test should follow one execution across these layers, including a Python failure, and check that concurrent executions do not share context. We do not need a span per tuple.
26ecde1 to
be6cf32
Compare
Introduce the OpenTelemetry foundation for Texera: an SDK bootstrap, a Logback-to-OTel log bridge, a log sanitizer, and per-request MDC enrichment, wired into every service entry point. This is PR 1 of the observability stack; it ships logging only, with the trace and metric exporters wired but not yet emitted (those arrive in follow-up PRs). New module (common/observability): - OtelInit: one-call SDK bootstrap per service. Reads OTEL_* settings from observability.conf, validates the OTLP endpoint (scheme + host allowlist) before any exporter is built, wires the span/log/metric providers explicitly (no sdk-extension-autoconfigure), clamps the metric export interval, and attaches the log appender to the Logback ROOT logger. The whole init body is guarded so a missing or malformed config returns None with one WARN instead of throwing into a service's run(), even when telemetry is disabled. - TexeraOtelLogAppender: bridges Logback events to OTel log records. Maps severity, forwards MDC, sets exception.type/message/stacktrace semantic attributes, and drops records from io.opentelemetry loggers so export-failure diagnostics are not fed back to the collector that just failed. - LogSanitizer: strips C0 control characters, redacts secrets, and caps body length (MaxBodyChars) to keep individual records bounded. Config (common/config): - observability.conf holds the OTEL_* defaults, ObservabilityConfig reads it, and EnvironmentalVariable carries the ENV_OTEL_* entries. Request/user context (amber web): - RequestContextMdcFilter: a servlet filter that tags every log record emitted during request handling with the workflow, execution, and computing-unit id when one is present in the URL, a header, or a WebSocket query parameter, so exported logs can be filtered by workflow or computing unit downstream. - UserContextMdcFilter: a JAX-RS filter that tags records with the authenticated user id, registered after the JWT auth filter so the principal is available. Both keep their MDC keys out of LogSanitizer.DeniedMdcKeys so the OTel appender forwards them to the collector. Wiring: - build.sbt defines the Observability module and adds dependsOn(Observability) to the eight Dropwizard services. - Each service entry point calls OtelInit.init with its own service name, and each service config gains a logging block. - TexeraWebApplication registers the two MDC filters during run(). Deployment: - OTEL_* env entries live in bin/single-node/.env, bin/k8s/values.yaml, and bin/k8s/values-development.yaml (kept a name-for-name mirror). - LICENSE-binary manifests list the pinned OTel 1.50.0 jars. Tests: OtelInitSpec, TexeraOtelLogAppenderSpec, LogSanitizerSpec, ObservabilityConfigSpec, RequestContextMdcFilterSpec, and UserContextMdcFilterSpec cover endpoint validation, interval clamping, severity/MDC/exception mapping, self-diagnostic filtering, redaction, truncation, per-request context extraction, and the sanitizer MDC contract.
Build on the logging foundation (PR 1) with the backend emit path: workflow lifecycle metrics and a run-level setup trace span, plus the tracing/metrics primitives they use. This is PR 2 of the observability stack. Primitives (common/observability): - TexeraTracer: lazy accessor for the process tracer off GlobalOpenTelemetry. - SpanAttrs: typed AttributeKey constants so span attributes use standard keys rather than ad hoc strings. - WorkflowMetrics: the OTel instruments (start/completion/failure/cancellation counters and run-duration histogram) keyed by workflow kind. - TraceparentValidator: validates W3C traceparent headers before use. Emit path (amber): - WorkflowMetricsRecorder: single owner of the metric instruments. init() wires them once; onStart stamps a run's start; onStateChange records terminal counters and duration exactly once on the first transition into a terminal state (idempotent, safe to call on every transition). - WorkflowService.initExecutionService runs inside a run-level setup span so setup-path logs carry its trace id. The span covers only the synchronous setup; synchronous setup failures are recorded on it in the catch block, and the span is ended in finally. The async errorHandler deliberately does not touch the span: it is invoked after setup returns and the span has ended, so the failure is surfaced through the metadata store instead. - ExecutionStateStore.updateWorkflowState is the single chokepoint that feeds every state transition to WorkflowMetricsRecorder.onStateChange. - ComputingUnitMaster initializes the recorder at startup. All observability sources live under common/observability (the module from PR 1); the tracing/metrics classes are not duplicated into common/config. Tests: WorkflowMetricsSpec, SpanAttrsSpec, and TraceparentValidatorSpec, plus TexeraTracerSpec (the tracer accessor and current-context passthrough) and WorkflowMetricsRecorderSpec (the state-to-metric mapping against an in-memory meter: starts increment the start counter, COMPLETED and FAILED record a completion and a duration sample, KILLED records a cancellation with no duration, and non-terminal or already-terminal transitions record nothing). Review follow-ups addressed: span attributes use SpanAttrs keys instead of plain strings; the error handler no longer records onto a span that may have already ended (documented and handled via the metadata store). Stacked on PR 1 (obs/pr1/foundations).
be6cf32 to
53455f5
Compare
zuozhiw
left a comment
zuozhiw
left a comment
There was a problem hiding this comment.
good to merge, please fix the CI
What changes were proposed in this PR?
Adds the libraries services use to emit metrics and distributed traces. These stay dormant until the SDK is enabled in PR1.
TexeraMetrics: metric instruments for workflow lifecycle and throughput. The active-executions metric is modeled as an observable gauge read from the live registry, correcting the phantom-active count a manual up/down counter produced.TexeraTracerandSpanAttrs: helpers for creating spans and attaching standardized attributes.TraceparentValidator: parses and validates W3Ctraceparentheaders for context propagation.Configmodule; no behavior changes when telemetry is disabled.Any related issues, documentation, or discussions?
Closes: #5368
Part of #4070. Stacked on #5375.
How was this PR tested?
sbt scalafmtCheckAllpasses; compile and tests run in this PR's CI.Was this PR authored or co-authored using generative AI tooling?
Co-authored with Claude Opus 4.8 in compliance with ASF