Ma77Ball commented on code in PR #5376: URL: https://github.com/apache/texera/pull/5376#discussion_r3593111141
########## common/config/src/main/scala/org/apache/texera/observability/TexeraTracer.scala: ########## @@ -0,0 +1,84 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.observability + +import io.opentelemetry.api.GlobalOpenTelemetry +import io.opentelemetry.api.trace.{Span, SpanBuilder, StatusCode, Tracer} +import io.opentelemetry.context.{Context, Scope} + +/** + * Thin convenience wrapper around the global OTel tracer. + * + * Two reasons to go through this rather than calling + * ``GlobalOpenTelemetry.getTracer`` directly at every callsite: + * + * 1. Single instrumentation scope name (``org.apache.texera``) — so + * every Texera-produced span shows up under one logical scope in + * the backend, separable from anything emitted by transitive + * libraries. + * 2. One ergonomic ``withSpan`` API that handles exception → status, + * scope cleanup, and span end in a single try/finally. Callers + * don't have to remember the ceremony at every site. + * + * When the SDK is disabled, ``GlobalOpenTelemetry.getTracer`` returns + * a no-op tracer, so calling these methods is safe at any time. + */ +object TexeraTracer { + + private val InstrumentationScope = "org.apache.texera" + + def tracer: Tracer = GlobalOpenTelemetry.getTracer(InstrumentationScope) + + def spanBuilder(name: String): SpanBuilder = tracer.spanBuilder(name) + + /** + * Run ``block`` inside a fresh span; record exceptions, propagate + * the right span status, and ensure the span is ended exactly once. + * + * Use this for synchronous critical sections. For async (Future- + * returning) code paths use ``withAsyncSpan`` so the span doesn't + * close before the async work completes. + */ + def withSpan[T](name: String, configure: SpanBuilder => SpanBuilder = identity)( Review Comment: Dropped the withSpan wrapper. TexeraTracer now just exposes the tracer under our single instrumentation scope, and callsites start/end spans with the standard OTel API (kept a scaladoc example of that pattern). ########## common/config/src/main/scala/org/apache/texera/observability/TexeraMetrics.scala: ########## @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.observability + +import com.typesafe.scalalogging.LazyLogging +import io.opentelemetry.api.GlobalOpenTelemetry +import io.opentelemetry.api.common.{AttributeKey, Attributes} +import io.opentelemetry.api.metrics.Meter + +/** + * Strongly-typed façade for Texera-emitted metrics. + * + * Cardinality safety is enforced by the API surface, not by + * documentation: there is no public method that accepts an arbitrary + * string as a label key or value. The only labels that ever land on + * an instrument are the two enums [[Outcome]] and [[WorkflowKind]], + * each restricted to a fixed set. ``workflow.id`` / ``execution.id`` + * are deliberately NOT metric labels — per-execution detail belongs + * in traces and logs, joined on ``trace_id`` at query time. + * + * Histogram bucket bounds are hard-coded constants so they can't be + * coerced by request input. The OTel SDK applies its own default + * attribute-value-length cap to anything that does slip through. + */ +object TexeraMetrics extends LazyLogging { Review Comment: Renamed it to WorkflowMetrics. I also documented that this facade is only for the workflow-execution cluster and added an example of calling the OTel meter API directly at a call site, so people don't copy the facade by default. ########## common/config/src/main/scala/org/apache/texera/observability/SpanAttrs.scala: ########## @@ -0,0 +1,120 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.observability + +import io.opentelemetry.api.common.AttributeKey +import io.opentelemetry.api.trace.{Span, SpanBuilder} + +/** + * Thin helper for setting span attributes safely. + * + * Three rules: + * 1. Typed setters only — no public escape hatch for arbitrary + * untyped strings to land on a span as untrusted free text. + * 2. Free-text values are CRLF-stripped + capped at + * [[FreeTextMaxLen]] to prevent log/span forging via embedded + * newlines. + * 3. Operator IDs and workflow/execution IDs must match a strict + * character set — otherwise dropped silently (the operator + * identifier should be a stable internal value, not user free + * text). + */ +object SpanAttrs { + + /** Maximum length for free-text span attribute values. */ + val FreeTextMaxLen: Int = 256 + + /** Validates the shape we accept for operator IDs: alnum + `_.-`, + * 1–64 chars. Anything else is dropped (not coerced — we'd rather + * miss a label than leak an unbounded string into a span). + */ + private val OperatorIdPattern = "^[A-Za-z0-9_.\\-]{1,64}$".r.pattern + + // ---- Standard Texera correlation labels ------------------------------ + + val WorkflowId: AttributeKey[java.lang.Long] = AttributeKey.longKey("texera.workflow.id") + val ExecutionId: AttributeKey[java.lang.Long] = AttributeKey.longKey("texera.execution.id") + val ProjectId: AttributeKey[java.lang.Long] = AttributeKey.longKey("texera.project.id") + val UserId: AttributeKey[java.lang.Long] = AttributeKey.longKey("texera.user.id") + val OperatorId: AttributeKey[String] = AttributeKey.stringKey("texera.operator.id") + val OperatorName: AttributeKey[String] = AttributeKey.stringKey("texera.operator.name") + val Outcome: AttributeKey[String] = AttributeKey.stringKey("texera.outcome") + + // ---- Typed setters for SpanBuilder (used at span-start time) --------- + + def withWorkflowId(b: SpanBuilder, id: Long): SpanBuilder = Review Comment: Removed the setter helpers. Callers now set attributes via the OTel API directly; I kept the shared label keys so the attribute names stay consistent across callsites. ########## common/config/src/main/scala/org/apache/texera/observability/SpanAttrs.scala: ########## @@ -0,0 +1,120 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.texera.observability + +import io.opentelemetry.api.common.AttributeKey +import io.opentelemetry.api.trace.{Span, SpanBuilder} + +/** + * Thin helper for setting span attributes safely. + * + * Three rules: + * 1. Typed setters only — no public escape hatch for arbitrary + * untyped strings to land on a span as untrusted free text. + * 2. Free-text values are CRLF-stripped + capped at + * [[FreeTextMaxLen]] to prevent log/span forging via embedded + * newlines. + * 3. Operator IDs and workflow/execution IDs must match a strict + * character set — otherwise dropped silently (the operator + * identifier should be a stable internal value, not user free + * text). + */ +object SpanAttrs { + + /** Maximum length for free-text span attribute values. */ + val FreeTextMaxLen: Int = 256 + + /** Validates the shape we accept for operator IDs: alnum + `_.-`, + * 1–64 chars. Anything else is dropped (not coerced — we'd rather + * miss a label than leak an unbounded string into a span). + */ + private val OperatorIdPattern = "^[A-Za-z0-9_.\\-]{1,64}$".r.pattern + + // ---- Standard Texera correlation labels ------------------------------ + + val WorkflowId: AttributeKey[java.lang.Long] = AttributeKey.longKey("texera.workflow.id") Review Comment: Thanks, kept these as the shared label keys. ########## amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala: ########## @@ -183,6 +185,25 @@ class WorkflowService( userOpt: Option[User], sessionUri: URI ): Unit = { + TexeraTracer.withSpan( + "workflow.execute", Review Comment: Renamed the span to WorkflowService.initExecutionService (ClassName.methodName). ########## amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala: ########## @@ -183,6 +185,25 @@ class WorkflowService( userOpt: Option[User], sessionUri: URI ): Unit = { + TexeraTracer.withSpan( + "workflow.execute", + _.setAttribute("texera.workflow.id", workflowId.id.toString) + ) { span => + initExecutionServiceSpanned(req, userOpt, sessionUri, span) Review Comment: Removed the initExecutionServiceSpanned split, so no span is passed around. The function starts its own span and ends it in a finally. ########## amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala: ########## @@ -183,6 +185,25 @@ class WorkflowService( userOpt: Option[User], sessionUri: URI ): Unit = { + TexeraTracer.withSpan( + "workflow.execute", + _.setAttribute("texera.workflow.id", workflowId.id.toString) + ) { span => Review Comment: Good catch. The failure is caught in errorHandler and doesn't propagate, so I now record it onto the span there (recordException + ERROR status) instead of in an outer catch that never sees it. Your broader point about always instrumenting the root REST/WebSocket endpoints directly is a good one; I'll do that as its own change since it touches the entry-point layer. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
