[Ma77Ball]

What changes were proposed in this PR?

In one sentence: The OpenTelemetry foundation every later observability PR builds on: off by default and opt-in, and anything leaving the process is validated or bounded first.

Plumbing only. No dashboards, no metrics instrumentation, no API changes.

---

The three pieces

Component	Job
OtelInit	The single entry point. Reads its OTEL_* settings from observability.conf, validates them, builds the SDK, registers it globally.
TexeraOtelLogAppender	A Logback appender that mirrors app logs into OTel. A thin shim, all security logic lives in the sanitizer.
LogSanitizer	Pure functions: redact secrets, strip control chars, cap size, drop noisy MDC keys (and redact credential-named ones) before anything ships.

---

How a log record flows

flowchart TD
    A["logger.info(...)"] --> B[Logback ROOT]
    B --> C["console / file (always)"]
    B --> D{appender bound?}
    D -- no --> E[no-op]
    D -- yes --> F[LogSanitizer]
    F --> G["strip ctrl chars · redact secrets · cap 16K chars · filter MDC"]
    G --> H[OTLP exporter] --> I[collector]

Loading

If the appender is not bound (init off or not yet run), logs still flow to stdout/file untouched.

---

Bootstrap, and how it stays safe

flowchart TD
    A[Service.run] --> B{OTEL_SDK_DISABLED == false?}
    B -- no --> C[INFO, no-op]
    B -- yes --> D{endpoint valid?\nscheme + host allowlist}
    D -- no --> E[one WARN, no-op]
    D -- yes --> F[build SDK · attrs passthrough, service.name locked]
    F --> G[attach appender · shutdown hook]

Loading

• Default OFF. Inert until explicitly enabled with OTEL_SDK_DISABLED=false; while disabled no exporters start and nothing is emitted (per [Observability] OpenTelemetry logging foundations (SDK bootstrap, log bridge, sanitizer) #5367).
• Fails quietly, never crashes. Missing collector or bad config gives one WARN and a no-op, never a startup exception.
• No autoconfigure SPI, on purpose: filtering must run before any exporter is built, so we read every OTEL_* setting ourselves through observability.conf (HOCON defaults with ${?OTEL_*} overrides).

---

The reason it is its own PR: every output is validated or bounded

Guard	Blocks
Endpoint scheme + host allowlist	exfiltration to arbitrary protocols/hosts (scheme is http/https only; extend hosts via TEXERA_OTEL_ALLOWED_HOSTS)
Resource-attribute service.name lock	env overriding the service identity every record is keyed by (other attributes pass through)
MDC deny-list + credential-key redaction	noisy bridge fields and secret-named values reaching exports
Secret redaction	Bearer tokens, password=, AWS keys printed in logs (message and stack trace)
Control-char strip	CR/LF log forging from user strings
Body cap (16K chars via MaxBodyChars) + metric-interval fallback	runaway log lines / busy-loop exporter

Exception stack traces are appended to the log body, so Dropwizard 500s stay diagnosable from the dashboard instead of vanishing behind "Error handling a request: <id>".

---

Supporting wiring

• OTel 1.50.0 deps pinned once in the common/observability module; every service inherits them via dependsOn(Observability).
• One-line OtelInit.init(...) added to each service entry point.
• The five OTEL_* knobs are exposed through observability.conf, EnvironmentalVariable, and the single-node .env + k8s values.yaml so operators can find and tune them.
• Framework log caps (pekko, iceberg, hadoop, kafka, jetty, ...) pinned at WARN so future TRACE/DEBUG surfaces Texera code, not the framework firehose.

Any related issues, documentation, or discussions?

Closes: #5367
Part of #4070. Approach: #5355

How was this PR tested?

• Unit specs for all three components: LogSanitizerSpec (redaction, stripping, truncation, MDC deny-list + credential-key redaction), OtelInitSpec (endpoint validation, resource-attribute passthrough with service.name protected, interval fallback, default-off, in-memory exporter), TexeraOtelLogAppenderSpec (throwable-body redaction, severity mapping, MDC handling), plus ObservabilityConfigSpec for the config defaults.
• sbt scalafmtCheckAll passes and the Config + Observability modules compile; full compile and tests run in CI.

Was this PR authored or co-authored using generative AI tooling?

Co-authored with Claude Opus 4.8 in compliance with ASF policy.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 78.16594% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.81%. Comparing base (d108cc1) to head (0737f9e).

Files with missing lines	Patch %	Lines
...ala/org/apache/texera/observability/OtelInit.scala	71.09%	25 Missing and 12 partials ⚠️
...e/texera/observability/TexeraOtelLogAppender.scala	78.04%	2 Missing and 7 partials ⚠️
...org/apache/texera/observability/LogSanitizer.scala	92.50%	0 Missing and 3 partials ⚠️
.../scala/org/apache/texera/service/FileService.scala	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5375      +/-   ##
============================================
- Coverage     91.87%   91.81%   -0.06%     
- Complexity     5048     5064      +16     
============================================
  Files          1265     1269       +4     
  Lines         54915    55144     +229     
  Branches       6940     6984      +44     
============================================
+ Hits          50452    50631     +179     
- Misses         2769     2796      +27     
- Partials       1694     1717      +23     

Flag	Coverage Δ
access-control-service	77.44% <100.00%> (+0.06%)	⬆️
agent-service	99.16% <ø> (ø)
amber	85.97% <78.02%> (-0.09%)	⬇️
computing-unit-managing-service	60.52% <100.00%> (+0.03%)	⬆️
config-service	87.50% <100.00%> (+0.12%)	⬆️
file-service	81.92% <0.00%> (-0.09%)	⬇️
frontend	96.49% <ø> (-0.01%)	⬇️
notebook-migration-service	83.77% <100.00%> (+0.03%)	⬆️
pyamber	98.54% <ø> (ø)
workflow-compiling-service	72.61% <100.00%> (+0.17%)	⬆️

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

⚠️ Benchmark changes need a look

🟢 2 better · 🔴 4 worse · ⚪ 9 noise (<±5%) · 0 without baseline

Compared against main d108cc1 benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

	config	throughput	MB/s	latency	max Δ latest / 7d
🔴	bs=10 sw=10 sl=64	427	0.26	22,406/33,866/33,866 us	🔴 +12.9% / 🔴 +137.9%
🔴	bs=100 sw=10 sl=64	946	0.577	104,459/120,286/120,286 us	🟢 -9.9% / 🔴 +24.7%
⚪	bs=1000 sw=10 sl=64	1,097	0.669	906,565/971,563/971,563 us	⚪ within ±5% / 🔴 -7.3%

Baseline details

Latest main d108cc1 from same runner

config	metric	PR	latest main	7d avg	Δ latest	Δ 7d
bs=10 sw=10 sl=64	throughput	427 tuples/sec	466 tuples/sec	879.16 tuples/sec	-8.4%	-51.4%
bs=10 sw=10 sl=64	MB/s	0.26 MB/s	0.284 MB/s	0.537 MB/s	-8.5%	-51.5%
bs=10 sw=10 sl=64	p50	22,406 us	19,843 us	11,504 us	+12.9%	+94.8%
bs=10 sw=10 sl=64	p95	33,866 us	32,903 us	14,234 us	+2.9%	+137.9%
bs=10 sw=10 sl=64	p99	33,866 us	32,903 us	17,338 us	+2.9%	+95.3%
bs=100 sw=10 sl=64	throughput	946 tuples/sec	971 tuples/sec	1,147 tuples/sec	-2.6%	-17.6%
bs=100 sw=10 sl=64	MB/s	0.577 MB/s	0.593 MB/s	0.7 MB/s	-2.7%	-17.6%
bs=100 sw=10 sl=64	p50	104,459 us	99,204 us	88,987 us	+5.3%	+17.4%
bs=100 sw=10 sl=64	p95	120,286 us	133,439 us	96,473 us	-9.9%	+24.7%
bs=100 sw=10 sl=64	p99	120,286 us	133,439 us	102,791 us	-9.9%	+17.0%
bs=1000 sw=10 sl=64	throughput	1,097 tuples/sec	1,104 tuples/sec	1,182 tuples/sec	-0.6%	-7.2%
bs=1000 sw=10 sl=64	MB/s	0.669 MB/s	0.674 MB/s	0.721 MB/s	-0.7%	-7.3%
bs=1000 sw=10 sl=64	p50	906,565 us	898,664 us	872,751 us	+0.9%	+3.9%
bs=1000 sw=10 sl=64	p95	971,563 us	992,905 us	910,151 us	-2.1%	+6.7%
bs=1000 sw=10 sl=64	p99	971,563 us	992,905 us	941,479 us	-2.1%	+3.2%

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,468.72,200,128000,427,0.260,22405.58,33866.26,33866.26
1,100,10,64,20,2115.14,2000,1280000,946,0.577,104458.98,120286.25,120286.25
2,1000,10,64,20,18234.63,20000,12800000,1097,0.669,906565.47,971562.92,971562.92

[github-actions]

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

• Committers with relevant context: @parshimers
  You can request their reviews formally with /request-review @parshimers.

• Contributors with relevant context: @bobbai00, @aicam, @xuang7
  You can notify them by mentioning @bobbai00, @aicam, @xuang7 in a comment.

[Ma77Ball]

/request-review @zuozhiw

[Ma77Ball]

/request-review @Yicong-Huang

[Yicong-Huang]

First round of comments: mainly on architecture

[Ma77Ball]

@zuozhiw, can you do a review?

[zuozhiw]

Approve the PR but left some comments. Please take a look and we can have a discussion.

From my perspective I don't really care about flow charts or architecture diagrams, I care more like what API does this provide to developers when they want to add logging, tracing, and metrics. So if there's a description of that it would be nice, if I missed it please let me know.

Also I only see a logging class being added. There are some trace and metrics related config, are we going to expose them as well?

[zuozhiw]

Approve the PR but left some comments. Please take a look and we can have a discussion.

From my perspective I don't really care about flow charts or architecture diagrams, I care more like what API does this provide to developers when they want to add logging, tracing, and metrics. So if there's a description of that it would be nice, if I missed it please let me know.

Also I only see a logging class being added. There are some trace and metrics related config, are we going to expose them as well?

Ok I now see the trace and metrics related stuff are in second PR, I'll continue to review that.

[zuozhiw]

I think this PR is in a good shape and it's good to merge @Ma77Ball

[Ma77Ball]

Thank you, @zuozhiw, for the review. I will continue working on the other observability PRs. @Yicong-Huang, do you want to do a second pass, or can this be merged?

[Yicong-Huang]

🔴 5 must-fix · 2 advisory · 2 polish — solid foundation; four gaps CI on this head cannot see, plus a default that contradicts the issue it closes.

Correctness (3)

• TexeraOtelLogAppender.scala:85 — stack traces bypass secret redaction (must-fix, see inline)
• OtelInit.scala:52 — grpc:// passes the allowlist but the OTLP exporter throws, aborting startup (must-fix, see inline)
• TexeraOtelLogAppenderSpec.scala:149 — this spec fails on the head; the deny-list exports secret/password MDC keys (must-fix, see inline)

Design & architecture (1)

• OtelInit.scala:156 — the five OTEL_* knobs reach no .conf, no EnvironmentalVariable entry and no deploy file, while the SDK is on by default (must-fix, see inline)

Conventions (2)

• Closing issue #5367 specifies default-off and "remains inert"; this PR ships default-on — flip the default or update the issue (must-fix)
• Description: the OTel deps are not in the Config module — they live in common/observability, and services use dependsOn(Observability) (advisory)

Simplifications (1)

• OtelInit.scala:58 — "::1" is unreachable; URI.getHost returns [::1] for the bracketed form (advisory, see inline)

Polish: 2 quick touch-ups (see inline comments).

Verification trace

Two load-bearing claims were checked against reality rather than docs. (1) The grpc:// crash: decompiled the pinned 1.50.0 jars — OtlpGrpcSpanExporterBuilder.setEndpoint delegates to ExporterBuilderUtil.validateEndpoint, which throws IllegalArgumentException for any scheme other than http/https, and the builder call at OtelInit.scala:184 sits outside any Try, so it escapes init() into run(). (2) The MDC gap: ran Observability/testOnly ...TexeraOtelLogAppenderSpec on JDK 17 — 7 passed, 1 failed with Set("secret", ..., "password", ...) contained element "secret", confirming key-named secrets are exported rather than dropped.

[Yicong-Huang]

🔴 9 resolved · 0 open · 9 new — every round-1 finding fixed at the source, not papered over. What's left is one theme: this feature's fan-out is missing three legs.

Design & architecture (5)

• amber/src/main/resources/web-config.yml:48 — TexeraWebApplication and NotebookMigrationService never call OtelInit.init (must-fix, see inline)
• bin/k8s/values.yaml:363 — values-development.yaml is missing all five OTEL_* entries (must-fix, see inline)
• OtelInit.scala:174 — the "never throws" contract has no top-level guard, and this read runs even when the SDK is disabled (must-fix, see inline)
• OtelInit.scala:395 — a ROOT-attached appender re-exports the exporter's own failure logs (must-fix, see inline)
• TexeraOtelLogAppender.scala:84 — the stack trace ships as body text, not as exception.* attributes (advisory, see inline)

Correctness (1)

• OtelInit.scala:351 — clampIntervalMs has no test, and its bounds constants were widened for tests that don't exist (must-fix, see inline)

Conventions (1)

• Description advertises four things the diff lacks: a resource-attribute allowlist, interval-fallback tests, init in every service, and a byte-based body cap (must-fix)

Polish: 2 quick touch-ups (see inline comments).

Verification trace

The log-feedback claim rests on third-party behavior, so I checked it. In the pinned OTel 1.50.0 jars, BatchLogRecordProcessor$Worker and ThrottlingLogger both reference java/util/logging, so export-failure diagnostics go to JUL — and org.slf4j.jul-to-slf4j is on every service's classpath (all six LICENSE-binary files), purely to install the bridge. That closes the path from a failed export back into the ROOT-attached appender.

The values-development.yaml omission was confirmed structurally: sorting both files' texeraEnvVars name lists yields 30 identical names, with these five as the only difference. That is why it reads as an oversight, not a slimmer dev profile.

Sub-classifying the nine new findings into newly-introduced vs. my own late catches was not possible — the prior review's head e69ca7a1 was force-pushed away and is no longer fetchable — so none are attributed either way.

[Ma77Ball]

@parshimers @sshiv012 @zuozhiw following the scope-down to instrumentation and export only, this PR is updated: it now also carries the two per-request MDC log-enrichment filters (RequestContextMdcFilter, UserContextMdcFilter) rescued from the dropped gateway PR, so exported logs carry the workflow, execution, computing-unit, and user id. Could you re-review when you get a chance? Thanks.

[Yicong-Huang]

🔴 0 resolved · 0 open · 7 new (7 new = 7 newly introduced · 0 late catches)

Design & architecture (1)

• TexeraWebApplication.scala:179: MDC filters registered only here, but their target routes live in other apps (must-fix, see inline)

Correctness (2)

• UserContextMdcFilter.scala:73: MDC cleared only in the response filter (advisory, see inline)
• RequestContextMdcFilter.scala:157: /api/executions/{wid}/... matches no pattern (advisory, see inline)

Conventions (1)

• TexeraWebApplication.scala:148: description omits the new MDC filters (advisory, see inline)

Polish: 4 quick touch-ups (see inline comments).

Verification trace

Traced the filter registration scope against the running apps: ComputingUnitMaster (a separate Dropwizard Application) registers SyncExecutionResource (/execution/{wid}/{cuid}/run) and the /wsapi/workflow-websocket endpoint but no MDC filter; computing-unit-managing-service serves /api/computing-unit/... and does not depend on amber, so the filter class is not on its classpath. On TexeraWebApplication only /workflow/<digits> URLs match a pattern. Verified the auth ordering: amber runs Dropwizard 1.3.23 (javax), so the new filter's imports are correct, and its AUTHORIZATION priority runs after the AUTHENTICATION-bucket auth filter.

[zuozhiw]

Thanks for moving the request-context work into this PR. Please address the process identity and request-context issues below before merging. They affect whether operators can trust the metrics and find the logs for a particular workflow execution.

Please also add unit tests corresponding to each issue.

[Yicong-Huang]

🔴 8 resolved · 0 open · 1 new (1 new = 1 newly introduced · 0 late catches)

Conventions (1)

• The Closes: #5367 link overclaims: the issue's proposed solution lists the per-request MDC filters, removed with no follow-up issue tracking the deferred tagging (must-fix). File and link the follow-up issue, or keep #5367 open.

Verification trace

The new service.instance.id is stable per process: init is guarded by a synchronized initialized.isDefined check (OtelInit.scala:77-85) and initInternal builds one Resource shared by the tracer, logger, and meter providers (OtelInit.scala:216-234), so the per-call UUID is minted once per JVM. Operator override works because parseAttrs applies OTEL_RESOURCE_ATTRIBUTES after the default put, excluding only service.name (verified by the new override test). The filter removal leaves no dangling references (tree-wide grep for the filter names returns zero hits).

[Ma77Ball]

@Yicong-Huang, ty for the re-review. I re-scoped the issue.

[Yicong-Huang]

🔴 1 resolved · 0 open · 3 new (3 new = 0 newly introduced · 3 late catches)

The Closes: #5367 concern from last round is resolved: the issue now defers per-request enrichment to a follow-up. The three new items are my own misses on code unchanged since earlier rounds.

Design & architecture (1)

• ComputingUnitMaster.scala:141: Kubernetes CU pods never receive the OTEL_* vars, so CU telemetry cannot be enabled (must-fix, see inline)

Conventions (1)

• access-control-service/LICENSE-binary:245: description heading does not match the template, so the template check reports the section missing (advisory, see inline)

Polish: 1 quick touch-up (see inline comments).

Verification trace

Traced how a Kubernetes computing unit gets its environment. ComputingUnitManagingResource calls KubernetesClient.createPod with computingUnitEnvironmentVariables plus the user token and JVM options; createPod builds the pod env from that map and the mounter vars only. That map is the iceberg and storage vars plus requiredComputingUnitEnvNames and optionalComputingUnitEnvNames, none of which is an OTEL_* name. Static services render texeraEnvVars from the chart and single-node uses env_file, so only CU pods miss the knobs.

[zuozhiw]

good to merge

I have checked the comments the major ones are fixed and the minor/nits don't need to be addressed