zuozhiw commented on code in PR #5376: URL: https://github.com/apache/texera/pull/5376#discussion_r3576393598
########## 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: Actually this file should be called workflow metrics, because it's essentially a helper class to group a few workflow execution related metrics. I'm not too against this pattern of adding a facade / helper class to group together a bunch of related metrics and expose simpler functions for the caller to use. This is fine for 1) complex metrics and 2) we do want centralized an standardize control. However in the meantime I do want to say that if all the metrics follow this facade pattern, then there way too much boilerplate for simple use cases, this metrics class, a recorder class, too much boilerplate. I would rather the call site to directly invoke the otel metrics apis and define the metrics close to the business logic, eg in many api handlers. Therefore we should promote that and add one or two examples to make sure that way of coding also works. I'm afraid that people will start to blindly copy this facade pattern, which makes sense for workflow execution related metrics but not all places. I actually believe most places do want just directly invoke the otel metrics apis. ########## 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: Honestly I don't believe our trace wrapper adds a ton of value here. This api is a bit awkward, maybe even a bit more complex than the official api. Please look at otel's official demo app to see what's the recommended way to use spans in Java code: https://github.com/open-telemetry/opentelemetry-demo/blob/main/src/ad/src/main/java/oteldemo/AdService.java https://opentelemetry.io/docs/demo/services/ad/ This file also contains metrics related code which is also otel officially recommended pattern, I suggest we follow these patterns. ########## 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: Another problem with this method is that the real error is actually not recorded into this span. This is because the child function has its own exception handler and the exception is already caught there I believe and won't propagate upward to here. The correct way is to make the actual function gets its own span instance via span.current in the beginning and make sure we have the corresponding span error handling code in the functions own error handling code. Apart from that I think our root level rest as well as websocket APIs should always have some span instrumentation to record things and start span directly in these endpoints, so that we know all the API calls and we can like put the response in the span if we wish, also the span duration naturally become the duration of this api call. If we need any kind of helpers or wrappers we should make those places always emit good spans. ########## 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: It's also not very common to directly pass a span into a function as a regular arg. If a function needs soan, it should do the span.current() to retrieve the current span, or if it wishes to start a new span (eg this function starts a substantial new work that worth tracking in a separate child span). I don't want our codebase to have such duplicate xxxSpannedfunctions. ########## 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: I believe this is span name, I don't think it's very common to use this naming convention. I think most commonly it's just function name, eg "WorkflowService.initWorkflow" so it should be like the function name in the end and the prefix is the enclosing files path. I believe this is common practice. Can you check otel official guidelines? Also check the otel demo repo ad service Java code that I put in another comment. ########## 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: Standard labels are good. ########## 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: These helpers also look a bit awkward we can't really do chain class on them, why not just let the callers call otel api? It's one line anyway -- 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]
