Copilot commented on code in PR #13742:
URL: https://github.com/apache/skywalking/pull/13742#discussion_r2924678237
##########
oap-server/analyzer/log-analyzer/CLAUDE.md:
##########
@@ -182,10 +182,11 @@ The generator detects the parser type from the AST at
compile time and generates
| JSON/YAML | `parsed.service` | `h.mapVal("service")` |
| JSON/YAML nested | `parsed.a.b` | `h.mapVal("a", "b")` |
| TEXT (regexp) | `parsed.level` | `h.group("level")` |
-| NONE + inputType | `parsed.response.code` | `((ExtraLogType)
h.ctx().extraLog()).getResponse().getCode()` |
-| NONE + no inputType | `parsed.service` | `h.ctx().log().getService()`
(LogData.Builder fallback) |
-| log fields | `log.service` | `h.ctx().log().getService()` |
-| log trace | `log.traceContext.traceId` |
`h.ctx().log().getTraceContext().getTraceId()` |
+| NONE + inputType | `parsed.response.code` | `((InputType)
h.ctx().input()).getResponse().getCode()` |
+| NONE + no inputType | `parsed.service` | `h.ctx().metadata().getService()`
(LogMetadata fallback) |
+| log fields (metadata) | `log.service` | `h.ctx().metadata().getService()` |
+| log fields (LogData) | `log.body` | `h.ctx().log().getBody()` |
Review Comment:
The table entry for `log.body` shows generated code
`h.ctx().log().getBody()`, but `ExecutionContext` no longer has `log()`. The
codegen in this PR uses `((LogData.Builder) h.ctx().input()).getBody()` for
LogData-only fields, so this doc example should be updated to match the new
runtime API.
```suggestion
| log fields (LogData) | `log.body` | `((LogData.Builder)
h.ctx().input()).getBody()` |
```
##########
test/script-cases/script-runtime-with-groovy/mal-lal-v1-v2-checker/src/test/java/org/apache/skywalking/oap/server/checker/lal/LalBenchmark.java:
##########
@@ -239,12 +239,14 @@ public void executeV1(final Blackhole bh) {
public void executeV2(final Blackhole bh) {
for (int i = 0; i < v2Exprs.size(); i++) {
try {
+ final LogData logData = testLogs.get(i);
+ final Message extraLog = extraLogs.get(i);
+ final org.apache.skywalking.oap.server.core.source.LogMetadata
metadata =
+
org.apache.skywalking.oap.server.core.source.LogMetadataUtils.fromLogData(logData);
+ final Object input = extraLog != null ? extraLog : logData;
final
org.apache.skywalking.oap.log.analyzer.v2.dsl.ExecutionContext ctx =
- new
org.apache.skywalking.oap.log.analyzer.v2.dsl.ExecutionContext()
- .log(testLogs.get(i));
- if (extraLogs.get(i) != null) {
- ctx.extraLog(extraLogs.get(i));
- }
+ new
org.apache.skywalking.oap.log.analyzer.v2.dsl.ExecutionContext();
+ ctx.init(metadata, input);
Review Comment:
In the v2 benchmark path, `input` is set to `logData` (an immutable
`LogData`) when there is no `extraLog`. The v2 runtime/codegen assumes
standard-log input is a `LogData.Builder` (e.g., `log.body`, `json{}`, `tag()`
all cast `ctx.input()` to `LogData.Builder`), so this will trigger
`ClassCastException` and the benchmark will silently ignore it due to the broad
catch, skewing results. Consider passing `logData.toBuilder()` (or storing
builders in `testLogs`) for the non-ALS case.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/spi/LALSourceTypeProvider.java:
##########
@@ -23,8 +23,8 @@
* SPI for receiver plugins to declare the input and default output types for
* LAL rules on a specific {@link Layer}.
*
- * <p><b>Input type</b> ({@link #inputType()}): The Java type of the {@code
extraLog}
- * passed to LAL via {@code ILogAnalyzerService.doAnalysis(LogData, Message)}.
+ * <p><b>Input type</b> ({@link #inputType()}): The Java type of the {@code
input}
+ * passed to LAL via {@code ILogAnalyzerService.doAnalysis(LogMetadata,
Object)}.
* The LAL compiler uses this at compile time to generate optimized direct
* getter calls for {@code parsed.*} field access. This is per-layer because
* all rules for a layer share the same input proto type.
Review Comment:
The updated Javadoc here references the new `(LogMetadata, Object)` input,
but later in this same comment block the “Input type resolution order” still
lists `LogData.Builder` as the final fallback. With the changes in this PR, the
fallback for no-parser/no-inputType `parsed.*` access is `LogMetadata`, so the
Javadoc should be updated for consistency.
##########
oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/source/LogMetadataUtils.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.skywalking.oap.server.core.source;
+
+import org.apache.skywalking.apm.network.logging.v3.LogData;
+import org.apache.skywalking.apm.network.logging.v3.TraceContext;
+
+/**
+ * Utility for building {@link LogMetadata} from protobuf {@link LogData}.
+ */
+public final class LogMetadataUtils {
+
+ private LogMetadataUtils() {
+ }
+
+ /**
+ * Build LogMetadata from a LogData protobuf message.
+ */
+ public static LogMetadata fromLogData(final LogData logData) {
+ LogMetadata.TraceContext tc = null;
+ if (logData.hasTraceContext()) {
+ final TraceContext ctxProto =
+ logData.getTraceContext();
+ tc = LogMetadata.TraceContext.builder()
+ .traceId(ctxProto.getTraceId())
+
.traceSegmentId(ctxProto.getTraceSegmentId())
+ .spanId(ctxProto.getSpanId())
+ .build();
+ }
+ return LogMetadata.builder()
+ .service(logData.getService())
+ .serviceInstance(logData.getServiceInstance())
+ .endpoint(logData.getEndpoint())
+ .layer(logData.getLayer())
+ .timestamp(logData.getTimestamp())
+ .traceContext(tc)
+ .build();
+ }
+
+ /**
+ * Build LogMetadata from a LogData.Builder.
+ */
+ public static LogMetadata fromLogData(final LogData.Builder builder) {
+ return fromLogData(builder.build());
Review Comment:
`fromLogData(LogData.Builder)` currently calls `builder.build()` and then
re-reads fields from the built message. In hot receiver paths this duplicates
the entire `LogData` (including potentially large body/tags) just to extract
metadata fields. Consider extracting directly from the builder (and its
traceContext builder/message) without building a full `LogData` instance.
```suggestion
LogMetadata.TraceContext tc = null;
if (builder.hasTraceContext()) {
final TraceContext.Builder ctxBuilder =
builder.getTraceContextBuilder();
tc = LogMetadata.TraceContext.builder()
.traceId(ctxBuilder.getTraceId())
.traceSegmentId(ctxBuilder.getTraceSegmentId())
.spanId(ctxBuilder.getSpanId())
.build();
}
return LogMetadata.builder()
.service(builder.getService())
.serviceInstance(builder.getServiceInstance())
.endpoint(builder.getEndpoint())
.layer(builder.getLayer())
.timestamp(builder.getTimestamp())
.traceContext(tc)
.build();
```
--
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]
