Repository navigation
Conversation
github-actions
Bot
added
dependencies
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5375 +/- ##
============================================
- Coverage 92.60% 92.54% -0.06%
- Complexity 5043 5060 +17
============================================
Files 1252 1256 +4
Lines 53587 53814 +227
Branches 6673 6717 +44
============================================
+ Hits 49623 49802 +179
- Misses 2315 2343 +28
- Partials 1649 1669 +20
☔ 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 | 393 | 0.24 | 24,657/30,096/30,096 us | 🔴 -9.8% / 🔴 +126.2% |
| 🔴 | bs=100 sw=10 sl=64 | 779 | 0.475 | 124,447/164,661/164,661 us | 🔴 +7.2% / 🔴 +76.5% |
| ⚪ | bs=1000 sw=10 sl=64 | 907 | 0.553 | 1,105,156/1,163,201/1,163,201 us | ⚪ within ±5% / 🔴 -30.3% |
Baseline details
Latest main be65bf8 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 393 tuples/sec | 435 tuples/sec | 983 tuples/sec | -9.7% | -60.0% |
| bs=10 sw=10 sl=64 | MB/s | 0.24 MB/s | 0.266 MB/s | 0.6 MB/s | -9.8% | -60.0% |
| bs=10 sw=10 sl=64 | p50 | 24,657 us | 23,360 us | 10,903 us | +5.6% | +126.2% |
| bs=10 sw=10 sl=64 | p95 | 30,096 us | 33,184 us | 13,539 us | -9.3% | +122.3% |
| bs=10 sw=10 sl=64 | p99 | 30,096 us | 33,184 us | 16,174 us | -9.3% | +86.1% |
| bs=100 sw=10 sl=64 | throughput | 779 tuples/sec | 834 tuples/sec | 1,259 tuples/sec | -6.6% | -38.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.475 MB/s | 0.509 MB/s | 0.769 MB/s | -6.7% | -38.2% |
| bs=100 sw=10 sl=64 | p50 | 124,447 us | 116,067 us | 86,981 us | +7.2% | +43.1% |
| bs=100 sw=10 sl=64 | p95 | 164,661 us | 157,126 us | 93,289 us | +4.8% | +76.5% |
| bs=100 sw=10 sl=64 | p99 | 164,661 us | 157,126 us | 105,695 us | +4.8% | +55.8% |
| bs=1000 sw=10 sl=64 | throughput | 907 tuples/sec | 931 tuples/sec | 1,300 tuples/sec | -2.6% | -30.2% |
| bs=1000 sw=10 sl=64 | MB/s | 0.553 MB/s | 0.568 MB/s | 0.793 MB/s | -2.6% | -30.3% |
| bs=1000 sw=10 sl=64 | p50 | 1,105,156 us | 1,075,155 us | 854,105 us | +2.8% | +29.4% |
| bs=1000 sw=10 sl=64 | p95 | 1,163,201 us | 1,134,756 us | 901,686 us | +2.5% | +29.0% |
| bs=1000 sw=10 sl=64 | p99 | 1,163,201 us | 1,134,756 us | 924,532 us | +2.5% | +25.8% |
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,508.66,200,128000,393,0.240,24657.02,30096.18,30096.18
1,100,10,64,20,2567.45,2000,1280000,779,0.475,124446.66,164661.36,164661.36
2,1000,10,64,20,22058.79,20000,12800000,907,0.553,1105156.22,1163201.27,1163201.27
Automated Reviewer SuggestionsBased on the
|
|
/request-review @zuozhiw |
|
/request-review @Yicong-Huang |
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
First round of comments: mainly on architecture
- Move OtelInit, TexeraOtelLogAppender, and
LogSanitizer (plus specs) from
common/config into a new common/observability
module; OTel deps are
declared once there and each service that calls
OtelInit.init now
depends on Observability explicitly.
- Revert common/config/build.sbt to its pre-PR
state, dropping the
incidental scalatest bump (back to 3.2.15).
- notebook-migration-service never inits OTel, so it
no longer bundles
the OTel jars (LICENSE-binary reverted).
- Document why AllowedMdcKeys forwards only Texera
correlation IDs and
what gets dropped.
- Add Observability/jacoco to the CI coverage job.
|
@zuozhiw, can you do a review? |
zuozhiw
left a comment
zuozhiw
left a comment
There was a problem hiding this comment.
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. |
…rough + 30s scrape Switch MDC and resource-attribute filtering from allow-list to deny-list so new correlation keys need no code edit (zuozhiw review): - LogSanitizer: replace AllowedMdcKeys with DeniedMdcKeys. Pass every key through except Pekko's noisy bridge keys, and run surviving values through sanitize() so secret-shaped content is still redacted. - OtelInit: buildResource now passes every OTEL_RESOURCE_ATTRIBUTES key through, keeping only service.name protected from override. - OtelInit: drop the default metric export interval from 60s to 30s for higher resolution. Update LogSanitizerSpec and OtelInitSpec to cover the new behavior.
|
I think this PR is in a good shape and it's good to merge @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
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 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 exportssecret/passwordMDC keys (must-fix, see inline)
Design & architecture (1)
OtelInit.scala:156— the five OTEL_* knobs reach no.conf, noEnvironmentalVariableentry 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
Configmodule — they live incommon/observability, and services usedependsOn(Observability)(advisory)
Simplifications (1)
OtelInit.scala:58—"::1"is unreachable;URI.getHostreturns[::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.
31d76c6 to
be75c16
Compare
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🔴 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—TexeraWebApplicationandNotebookMigrationServicenever callOtelInit.init(must-fix, see inline)bin/k8s/values.yaml:363—values-development.yamlis missing all fiveOTEL_*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 asexception.*attributes (advisory, see inline)
Correctness (1)
OtelInit.scala:351—clampIntervalMshas 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,
initin 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.
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🚫 Merge conflict with main — no review was performed on this revision.
This PR cannot merge in its current state, so its diff is not the change that would land. Please merge or rebase onto main, resolve the conflict, and re-request review.
|
This PR currently conflicts with the base branch. |
|
This PR conflicts with its base branch and needs to be rebased before review can continue. |
c5ee452 to
1ebcf66
Compare
1ebcf66 to
2ea65b9
Compare
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🚫 Merge conflict with main — no review was performed on this revision.
This PR cannot merge in its current state, so its diff is not the change that would land. Please merge or rebase onto main, resolve the conflict, and re-request review.
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🚫 Merge conflict with main — no review was performed on this revision.
This PR cannot merge in its current state, so its diff is not the change that would land. Please merge or rebase onto main, resolve the conflict, and re-request review.
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🚫 Merge conflict with main — no review was performed on this revision.
This PR cannot merge in its current state, so its diff is not the change that would land. Please merge or rebase onto main, resolve the conflict, and re-request review.
2ea65b9 to
2cd44ee
Compare
45ed6c7 to
c310922
Compare
Yicong-Huang
left a comment
Yicong-Huang
left a comment
There was a problem hiding this comment.
🟡 8 resolved · 1 open · 1 new (1 new = 0 newly introduced · 1 late catches)
Conventions (2)
OtelInit.scala:324: description claims a resource-attribute allowlist the code does not implement (advisory, see inline)NotebookMigrationService.scala:65: service config lacks the framework log caps the other services got (advisory, see inline)
Verification trace
Traced the three core components against the tree: OtelInit.buildResource applies every parsed OTEL_RESOURCE_ATTRIBUTES entry as passthrough (only service.name is protected), confirming the allowlist claim in the description is inaccurate. Confirmed all eight services now call OtelInit.init and the other eight web-config yamls carry the framework log caps; the notebook-migration-service config is the outlier.
| * here; the one exception is service.name, which the argument controls | ||
| * and env cannot override. | ||
| */ | ||
| private[observability] def buildResource(serviceName: String, rawAttrs: String): Resource = { |
There was a problem hiding this comment.
Advisory:
The description's guard table still claims a "Resource-attribute allowlist". buildResource applies every parsed OTEL_RESOURCE_ATTRIBUTES entry as passthrough (per the earlier review discussion), protecting only service.name from override. The table should say passthrough, not allowlist. (The same table's "Body cap (16 KiB)" is also slightly off: the cap is 16384 chars via MaxBodyChars, not bytes.)
There was a problem hiding this comment.
Yes, that description is outdated. Thanks for pointing this out. I will fix it shortly.
There was a problem hiding this comment.
Fixed the description. The guard table now says resource attributes pass through (only service.name is locked), not "allowlist", and the body cap reads "16K chars via MaxBodyChars" instead of "16 KiB". Updated the matching mermaid node and the test-coverage line too.
| configuration: NotebookMigrationServiceConfiguration, | ||
| environment: Environment | ||
| ): Unit = { | ||
| org.apache.texera.observability.OtelInit.init("notebook-migration-service") |
There was a problem hiding this comment.
Advisory:
This service now runs OtelInit.init, so its logs reach the collector, but notebook-migration-service-web-config.yaml lacks the framework log caps (org.apache.pekko, io.grpc, etc. at WARN) that the other eight service configs carry. Without them, a TRACE or DEBUG TEXERA_SERVICE_LOG_LEVEL forwards the framework firehose to the OTel backend. Add the same caps the other configs got when they landed.
There was a problem hiding this comment.
Added the same WARN log caps (org.apache.pekko, io.grpc, etc.) to notebook-migration-service-web-config.yaml that the other eight services have, so a DEBUG/TRACE log level no longer forwards the framework firehose to the collector.
Introduce the OpenTelemetry foundation for Texera: an SDK bootstrap, a Logback-to-OTel log bridge, and a log sanitizer, 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 with the OTEL_* defaults, ObservabilityConfig to read it, and ENV_OTEL_* entries in EnvironmentalVariable. 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. Deployment: - OTEL_* env entries in bin/single-node/.env, bin/k8s/values.yaml, and bin/k8s/values-development.yaml (kept a name-for-name mirror). - LICENSE-binary manifests updated with the pinned OTel 1.50.0 jars. Tests: OtelInitSpec, TexeraOtelLogAppenderSpec, LogSanitizerSpec, and ObservabilityConfigSpec cover endpoint validation, interval clamping, severity/MDC/exception mapping, self-diagnostic filtering, redaction, and truncation. Rebased onto current main.
c310922 to
f00c152
Compare
This branch has not been deployed
4 participants
What changes were proposed in this PR?
Plumbing only. No dashboards, no metrics instrumentation, no API changes.
The three pieces
OtelInitOTEL_*settings fromobservability.conf, validates them, builds the SDK, registers it globally.TexeraOtelLogAppenderLogSanitizerHow 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]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]OTEL_SDK_DISABLED=false; while disabled no exporters start and nothing is emitted (per [Observability] OpenTelemetry logging foundations (SDK bootstrap, log bridge, sanitizer) #5367).OTEL_*setting ourselves throughobservability.conf(HOCON defaults with${?OTEL_*}overrides).The reason it is its own PR: every output is validated or bounded
http/httpsonly; extend hosts viaTEXERA_OTEL_ALLOWED_HOSTS)service.namelockpassword=, AWS keys printed in logs (message and stack trace)MaxBodyChars) + metric-interval fallbackException 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
1.50.0deps pinned once in thecommon/observabilitymodule; every service inherits them viadependsOn(Observability).OtelInit.init(...)added to each service entry point.OTEL_*knobs are exposed throughobservability.conf,EnvironmentalVariable, and the single-node.env+ k8svalues.yamlso operators can find and tune them.pekko,iceberg,hadoop,kafka,jetty, ...) pinned atWARNso 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?
LogSanitizerSpec(redaction, stripping, truncation, MDC deny-list + credential-key redaction),OtelInitSpec(endpoint validation, resource-attribute passthrough withservice.nameprotected, interval fallback, default-off, in-memory exporter),TexeraOtelLogAppenderSpec(throwable-body redaction, severity mapping, MDC handling), plusObservabilityConfigSpecfor the config defaults.sbt scalafmtCheckAllpasses and theConfig+Observabilitymodules 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.