From 2e8df3e67e413dc376f379c358ffa596a78ab790 Mon Sep 17 00:00:00 2001 From: Wu Sheng Date: Sun, 9 Aug 2026 18:04:19 +0800 Subject: [PATCH] Fix generated-class source attribution in MAL, LAL and OAL When generated DSL code throws, the stack frame should name the file it came from, the line inside that file, and the line of the rule that produced it. All three were broken, in eight places across three compilers, each producing a number that was wrong and looked right. at ...rt.envoy_ca_L41_service_listener_ssl_ca_expiration_seconds.run( envoy_ca_L41_service_listener_ssl_ca_expiration_seconds.java:12) | | | envoy-ca.yaml line 41 a real file line 12 - The _L_ segment was the rules-list INDEX, not a line, so every file's first rule read L0. It now carries the rule's real YAML line. - SourceFile named a file that was never written, so source-attach could not resolve a frame regardless of the line. It now names the sidecar on disk. - LineNumberTable held statement ORDINALS, matching neither the YAML nor the generated source. MAL now maps each statement to its real sidecar line. Fixing any two of the three leaves the frame useless, which is why the class naming issue is part of this rather than cosmetic. Closure companions were the gap: a companion always CLAIMED a source file, because Javassist stamps SourceFile unconditionally, but the file was never written -- 84 dangling references in network-profiling alone. Each now writes its own sidecar and carries one entry at its SAM signature. One entry is derived, not conceded: the statement scanner marks boundaries at stores to a result slot, which a closure body never makes. LAL and OAL had the same defect and are fixed the same way, going from 0/48 resolvable to 48/48 for LAL. Their line tables are REMOVED rather than corrected: their statements are void invocations that store nothing, so no sound boundary detector exists, and an ordinal that resolves is worse than an absent attribute. A single signature entry for them is a follow-up. Hierarchy and OAL's metrics-builder write no sidecar at all, so their SourceFile still addresses nothing; both are recorded rather than half-fixed. Both sidecar writers also moved off a platform-default FileWriter with a non-ASCII header -- the pairing that produced MalformedInputException for MAL on this branch. Scope is deliberately the error path only. No API, no payload, no format change: the dsl-debugging probes, records and UI are untouched, and their line numbers come from parse-time tokens that never involved bytecode. --- docs/en/changes/changes.md | 6 + .../dynamic-code-generation-debugging.md | 59 +++-- .../backend/admin-api/dsl-debugging-mal.md | 4 +- .../v2/compiler/LALClassGenerator.java | 99 ++------- .../LALSourceAttributionScriptTest.java | 204 +++++++++++++++++ .../compiler/LALSourceFileResolvesTest.java | 110 ++++++++++ .../feature-cases/source-attribution.yaml | 70 ++++++ oap-server/analyzer/meter-analyzer/CLAUDE.md | 16 ++ .../meter/analyzer/v2/MalYamlLineIndex.java | 192 ++++++++++++++++ .../oap/meter/analyzer/v2/MetricConvert.java | 14 +- .../meter/analyzer/v2/MetricRuleConfig.java | 42 ++++ .../v2/compiler/MALBytecodeHelper.java | 194 ++++++++++++++--- .../v2/compiler/MALClassGenerator.java | 21 +- .../v2/compiler/MALClosureCodegen.java | 50 ++++- .../v2/compiler/MalGeneratedSourceLines.java | 89 ++++++++ .../analyzer/v2/compiler/MalSourceRef.java | 206 ++++++++++++++++++ .../v2/prometheus/rule/MetricsRule.java | 14 ++ .../analyzer/v2/prometheus/rule/Rule.java | 16 ++ .../v2/prometheus/rule/RuleSourceLines.java | 71 ++++++ .../analyzer/v2/prometheus/rule/Rules.java | 9 +- .../analyzer/v2/MalYamlLineIndexTest.java | 127 +++++++++++ .../v2/compiler/MalClassAttributes.java | 84 +++++++ .../MalClosureLineAttributionTest.java | 175 +++++++++++++++ .../v2/compiler/MalCompanionSourceTest.java | 180 +++++++++++++++ .../compiler/MalGeneratedSourceLinesTest.java | 92 ++++++++ .../v2/compiler/MalLineAttributionTest.java | 181 +++++++++++++++ .../v2/compiler/MalSourceRefTest.java | 79 +++++++ .../oal/v2/generator/OALClassGeneratorV2.java | 152 ++++++------- .../runtimerule/apply/MalFileApplier.java | 4 + .../server/starter/DSLClassGeneratorTest.java | 14 +- 30 files changed, 2338 insertions(+), 236 deletions(-) create mode 100644 oap-server/analyzer/log-analyzer/src/test/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALSourceAttributionScriptTest.java create mode 100644 oap-server/analyzer/log-analyzer/src/test/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALSourceFileResolvesTest.java create mode 100644 oap-server/analyzer/log-analyzer/src/test/resources/scripts/lal/test-lal/feature-cases/source-attribution.yaml create mode 100644 oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MalYamlLineIndex.java create mode 100644 oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalGeneratedSourceLines.java create mode 100644 oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalSourceRef.java create mode 100644 oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/RuleSourceLines.java create mode 100644 oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/MalYamlLineIndexTest.java create mode 100644 oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalClassAttributes.java create mode 100644 oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalClosureLineAttributionTest.java create mode 100644 oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalCompanionSourceTest.java create mode 100644 oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalGeneratedSourceLinesTest.java create mode 100644 oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalLineAttributionTest.java create mode 100644 oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalSourceRefTest.java diff --git a/docs/en/changes/changes.md b/docs/en/changes/changes.md index bd571522c2c1..5087a53b249a 100644 --- a/docs/en/changes/changes.md +++ b/docs/en/changes/changes.md @@ -255,6 +255,12 @@ admin-host only" entry above for the public REST retirement. #### OAP Server +* Fix MAL generated-class line attribution, which conflated three different coordinate spaces. + - The `_L_` segment in a generated class name was the rules-list index, not a line, so every rule file's first rule was reported as `L0`. It now carries the rule's real YAML line (and the `filter:` line for filter classes). + - The `SourceFile` attribute named a file that was never written — it used the metric name while the generated source file is named after the class — so IDE source-attach could never resolve a MAL stack frame regardless of the line. It now equals the generated `.java` file. + - `LineNumberTable` held statement ordinals matching neither the YAML nor the generated source; it now points at real statements in the generated `.java`. + - Closure companion classes now get their own `.java` and their own line. Javassist always stamps a companion's `SourceFile` as `.java`, but that file was never written, so every closure frame named a source file that did not exist. Each companion is a separate class file — Javassist cannot emit lambdas, so one rule with two closures produces three classes — and each now carries its own source file plus a single `LineNumberTable` entry at its SAM signature. One entry rather than a per-statement table is deliberate: the statement scanner detects stores to a result slot, which a closure body never makes, so a per-statement table would be invented rather than derived. + - The same `SourceFile` defect is fixed in LAL and OAL, which stamped the rule's YAML provenance (`(execution-basic.yaml:304)auto-layer-not-set.java`) while writing `execution_basic_L304_auto_layer_not_set.java`, so no generated frame in either DSL could ever resolve. Their `.java` sidecars are also now written as UTF-8 with an ASCII header instead of through a platform-default `FileWriter`, which is the pairing that produced a `MalformedInputException` for MAL on a non-UTF-8 JVM. Hierarchy writes no sidecar at all, so its `SourceFile` still names nothing — tracked separately, since adding one is a feature rather than a fix. * Support runtime rule hot-update and DSL debugging for the `meter-analyzer-config` catalog, bringing native meter (`MeterReportService`) rules to parity with `otel-rules`. - Meter rules now load through the same `Rules`/`Rule` pipeline `otel-rules` uses, so they participate in `RuleSetMerger`, are recorded in `StaticRuleRegistry`, support the optional `layerDefinitions` block, and generate source-named expression classes instead of falling back to `MalExpr_`. - `MeterProcessService` now implements `MalConverterRegistry` and publishes debug holders at boot, so a meter rule can be added / overridden / inactivated at runtime, and attached to a DSL debug session, without restarting the OAP. diff --git a/docs/en/operation/dynamic-code-generation-debugging.md b/docs/en/operation/dynamic-code-generation-debugging.md index b582f81df103..96cdf1c1f4c3 100644 --- a/docs/en/operation/dynamic-code-generation-debugging.md +++ b/docs/en/operation/dynamic-code-generation-debugging.md @@ -98,17 +98,39 @@ When a runtime error occurs inside a generated class, the JVM prints a stack tra at ..(SourceFile:LineNumber) ``` -The `SourceFile` attribute encodes the original DSL configuration file in parentheses: +The `.java` sidecar is written ON DEMAND only — when `SW_DYNAMIC_CLASS_ENGINE_DEBUG` is set. +Javassist compiles from an in-memory string, so a default deployment produces classes with no +sidecar on disk at all. + +When a sidecar IS written, `SourceFile` names it, so an IDE can resolve the frame to real source: ``` -(:).java +.java ``` +When none is written, the frame's usefulness depends on the DSL. MAL and LAL encode the rule's line +in the CLASS NAME (`vm_L38_cpu_total`, `execution_basic_L304_…`), so the location survives regardless. +An OAL class is named after its metric (`ServiceRespTimeMetrics`) and carries no line elsewhere, so +its `SourceFile` keeps a `(core.oal:20)` prefix outside debug mode — and always, for the +metrics-builder and Hierarchy classes, which write no sidecar in any mode. A dispatcher is shared by +every metric of a scope, so it names no line at all rather than borrow one metric's. + +For MAL and LAL it no longer embeds the DSL location in parentheses. That form described where a rule came from but +addressed no file on disk, so source-attach could never resolve a frame. The DSL location is instead +carried by the generated class NAME (`vm_L38_cpu_total` is line 38 of `vm.yaml`). + +Line numbers differ by DSL. MAL attaches a `LineNumberTable` mapping each statement to its line in +the sidecar, and one entry at the SAM signature for closure companions. LAL and OAL attach no line +table: theirs numbered statements 1, 2, 3 rather than addressing sidecar lines, and once `SourceFile` +started resolving, those ordinals resolved too — to the sidecar's comment header. An absent table +makes the JVM report an unknown line, which is honest; real lines there require the sidecar geometry +to be derived and tested, which is not yet done. + ### Example Stack Trace ``` java.lang.ArithmeticException: / by zero - at ...metrics.generated.ServiceRespTimeMetrics.id0((core.oal:20)ServiceRespTimeMetrics.java:3) + at ...metrics.generated.ServiceRespTimeMetrics.id0(ServiceRespTimeMetrics.java:3) at ...worker.MetricsStreamProcessor.in(MetricsStreamProcessor.java:...) ... ``` @@ -122,17 +144,18 @@ Reading this: | DSL | SourceFile Example | Generated Class Name | How to Read | |-----|-------------------|---------------------|-------------| -| OAL | `(core.oal:20)ServiceRespTimeMetrics.java` | `ServiceRespTimeMetrics` | OAL file `core.oal`, line 20 defines this metric | -| MAL | `(vm.yaml:25)cpu_total_percentage.java` | `vm_L25_cpu_total_percentage` | YAML file `vm.yaml`, line 25, rule `cpu_total_percentage` | -| MAL filter | `(vm.yaml:20)filter.java` | `vm_L20_filter` | YAML file `vm.yaml`, line 20, filter expression | -| LAL | `(default.yaml:3)default.java` | `default_L3_default` | YAML file `default.yaml`, line 3, rule `default` | -| Hierarchy | `(hierarchy-definition.yml:88)name.java` | `hierarchy_definition_L88_name` | Rule `name` at line 88 in `hierarchy-definition.yml` | +| OAL | `ServiceRespTimeMetrics.java` | `ServiceRespTimeMetrics` | OAL file `core.oal`, line 20 defines this metric | +| MAL | `vm_L25_cpu_total_percentage.java` | `vm_L25_cpu_total_percentage` | YAML file `vm.yaml`, line 25, rule `cpu_total_percentage`. The `SourceFile` equals the generated class file so an IDE can resolve the frame; the YAML provenance is carried by the class name. | +| MAL filter | `vm_L20_filter.java` | `vm_L20_filter` | YAML file `vm.yaml`, line 20, filter expression | +| LAL | `default.java` | `default_L3_default` | YAML file `default.yaml`, line 3, rule `default` | +| Hierarchy | `name.java` | `hierarchy_definition_L88_name` | Rule `name` at line 88 in `hierarchy-definition.yml` | **Notes:** - The class name pattern is `{yamlFileName}_L{lineNo}_{ruleName}` for all DSLs (except OAL). The yaml file name and line number from `yamlSource` are combined with the rule name or `filter`. -- The number after `:` in the SourceFile prefix is the line number in the YAML file where the rule is defined (for MAL in production, this may be a 0-based rule index instead of a line number). -- When source information is unavailable, the class name falls back to `MalExpr_` / `LalExpr_` / `HierarchyRule_` and the SourceFile to just `ClassName.java` without the parenthesized prefix. +- The number after `_L` in the class name is the 1-based line in the YAML file where the rule (or the `filter:`) is defined. +- For MAL, the line number in a stack frame (`SourceFile:LineNumber`) is a line in the GENERATED `.java` file, not in the YAML. The two are different coordinate spaces: use the class name for YAML provenance, and the frame line to locate the generated statement. The per-stage YAML line is reported separately by the [DSL debugging API](../setup/backend/admin-api/dsl-debugging-mal.md) as `sourceLine`. +- When source information is unavailable, the class name falls back to `MalExpr_` / `LalExpr_` / `HierarchyRule_`. For MAL, a line that was expected but could not be resolved renders as `_Lunknown_` rather than being silently omitted, so the failure stays visible. ### Mapping Back to DSL Source @@ -142,13 +165,17 @@ Reading this: - `...log.analyzer.v2.compiler.rt.*` → LAL (class name: `{yamlName}_L{lineNo}_{ruleName}` or `LalExpr_`) - `...hierarchy.rule.rt.*` → Hierarchy (class name: `{yamlName}_L{lineNo}_{ruleName}` or `HierarchyRule_`) -2. **Find the source file** from the parenthesized prefix in the SourceFile attribute (e.g., `core.oal`, `vm.yaml`), - or from the class name prefix (e.g., `vm_L25_...` → `vm.yaml`). These files are in the `config/` directory. +2. **Find the source file** from the class name prefix (e.g., `vm_L25_...` → `vm.yaml`). These files are + in the `config/` directory. OAL classes are named after the metric rather than the file, so their + `SourceFile` keeps a `(core.oal:20)` prefix when no sidecar was written — that is the only place an + OAL frame carries its rule location. -3. **Locate the rule** using the line number from the class name (`_L25_`) or the SourceFile prefix (`:25`). +3. **Locate the rule** using the line number in the class name (`_L25_`), or the `(file:line)` prefix + for OAL. -4. **Use the statement number** (after the last `:`) as a rough indicator of which operation within the - generated method failed. Dump the class (see above) and use `javap -v` to see the exact mapping. +4. **Use the line number after the last `:`** to find the statement inside the generated `.java`. MAL + frames carry a real line there; LAL and OAL frames carry none, so they identify the method but not + the statement. Dump the class (see above) and use `javap -v` to inspect the mapping. ## Generating All DSL Classes Offline @@ -207,7 +234,7 @@ MAL and LAL errors during metric processing are caught and logged per-expression ``` ERROR o.a.s.o.m.a.v.MetricConvert - Analyze Analyzer{...} error java.lang.NullPointerException - at ...vm_L25_cpu_total_percentage.run((vm.yaml:25)cpu_total_percentage.java:5) + at ...vm_L25_cpu_total_percentage.run(cpu_total_percentage.java:5) ``` This tells you: the error is in `vm.yaml`, line 25, metric `cpu_total_percentage`, diff --git a/docs/en/setup/backend/admin-api/dsl-debugging-mal.md b/docs/en/setup/backend/admin-api/dsl-debugging-mal.md index 7c096a43b487..c39689399820 100644 --- a/docs/en/setup/backend/admin-api/dsl-debugging-mal.md +++ b/docs/en/setup/backend/admin-api/dsl-debugging-mal.md @@ -27,7 +27,9 @@ nodes[] sourceText — verbatim DSL fragment for this probe continueOn — true (MAL captures kept-only; see overview) payload — SampleFamily.toJson() at this probe stage - sourceLine — omitted for MAL (no per-line mapping) + written on. Per STAGE, not per rule: a stage from the + file-level `expSuffix:` reports the suffix's line, not + the rule's. Omitted when it could not be resolved. ``` Sample types and the probes that emit them: diff --git a/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALClassGenerator.java b/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALClassGenerator.java index 5453b29d332b..2267da95a81d 100644 --- a/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALClassGenerator.java +++ b/oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALClassGenerator.java @@ -21,6 +21,9 @@ import java.io.File; import java.io.FileOutputStream; import java.io.IOException; +import java.io.OutputStreamWriter; +import java.io.Writer; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -373,64 +376,6 @@ private void writeClassFile(final CtClass ctClass) { } } - /** - * Adds a {@code LineNumberTable} attribute by scanning bytecode for - * store instructions to local variable slots ≥ {@code firstResultSlot}. - */ - private void addLineNumberTable(final javassist.CtMethod method, - final int firstResultSlot) { - try { - final javassist.bytecode.MethodInfo mi = method.getMethodInfo(); - final javassist.bytecode.CodeAttribute code = mi.getCodeAttribute(); - if (code == null) { - return; - } - - final List entries = new ArrayList<>(); - int line = 1; - boolean nextIsNewLine = true; - - final javassist.bytecode.CodeIterator ci = code.iterator(); - while (ci.hasNext()) { - final int pc = ci.next(); - if (nextIsNewLine) { - entries.add(new int[]{pc, line++}); - nextIsNewLine = false; - } - final int op = ci.byteAt(pc) & 0xFF; - int slot = -1; - if (op >= 59 && op <= 78) { - slot = (op - 59) % 4; - } else if (op >= 54 && op <= 58) { - slot = ci.byteAt(pc + 1) & 0xFF; - } - if (slot >= firstResultSlot) { - nextIsNewLine = true; - } - } - - if (entries.isEmpty()) { - return; - } - - final javassist.bytecode.ConstPool cp = mi.getConstPool(); - final byte[] info = new byte[2 + entries.size() * 4]; - info[0] = (byte) (entries.size() >> 8); - info[1] = (byte) entries.size(); - for (int i = 0; i < entries.size(); i++) { - final int off = 2 + i * 4; - info[off] = (byte) (entries.get(i)[0] >> 8); - info[off + 1] = (byte) entries.get(i)[0]; - info[off + 2] = (byte) (entries.get(i)[1] >> 8); - info[off + 3] = (byte) entries.get(i)[1]; - } - code.getAttributes().add( - new javassist.bytecode.AttributeInfo(cp, "LineNumberTable", info)); - } catch (Exception e) { - log.warn("Failed to add LineNumberTable: {}", e.getMessage()); - } - } - private static void setSourceFile(final CtClass ctClass, final String name) { try { final javassist.bytecode.ClassFile cf = ctClass.getClassFile(); @@ -445,19 +390,6 @@ private static void setSourceFile(final CtClass ctClass, final String name) { } } - /** - * Builds the SourceFile name for a generated class. When YAML source info - * is available, produces {@code "default(ruleName.java)"}; - * otherwise falls back to {@code "ruleName.java"}. - */ - private String formatSourceFileName(final String ruleName) { - final String classFile = ruleName + ".java"; - if (yamlSource != null) { - return "(" + yamlSource + ")" + classFile; - } - return classFile; - } - private void addLocalVariableTable(final javassist.CtMethod method, final String className, final String[][] vars) { @@ -569,7 +501,6 @@ public LalExpression compileFromModel(final LALScriptModel model) throws Excepti final javassist.CtMethod ctMethod = CtNewMethod.make(pm.source, ctClass); ctClass.addMethod(ctMethod); addLocalVariableTable(ctMethod, className, pm.lvtVars); - addLineNumberTable(ctMethod, pm.lvtVars.length + 1); // after this + params } final javassist.CtMethod execMethod = CtNewMethod.make(executeBody, ctClass); @@ -590,10 +521,19 @@ public LalExpression compileFromModel(final LALScriptModel model) throws Excepti execLvt.addAll(genCtx.localVarLvtVars); addLocalVariableTable(execMethod, className, execLvt.toArray(new String[0][])); - addLineNumberTable(execMethod, 3); // slot 0=this, 1=filterSpec, 2=ctx - - setSourceFile(ctClass, formatSourceFileName( - classNameHint != null ? classNameHint : className)); + // No LineNumberTable. It numbered bytecode boundaries 1, 2, 3 -- statement ORDINALS, + // not lines in any file. That was harmless only while SourceFile named a file that did + // not exist, so no frame could resolve and act on the wrong number. Now that SourceFile + // names the sidecar actually written, an ordinal RESOLVES: line 1 of that file is the + // synthetic comment header, which an IDE presents as the frame's source. Emitting real + // lines needs the sidecar's geometry (preamble + package + class decl + preceding private + // methods, which vary per rule) the way MAL derives it; until that is computed and + // tested, an absent attribute reports an unknown line, which is honest. + + // Must equal the .java written below: a SourceFile naming a file that does not exist + // reads as correct to an IDE right up until it fails to open it. YAML provenance + // belongs in the debug API, not in this attribute. + setSourceFile(ctClass, ctClass.getSimpleName() + ".java"); writeClassFile(ctClass); writeSourceFile(ctClass, genCtx, executeBody); @@ -623,7 +563,7 @@ private void writeSourceFile(final CtClass ctClass, } final File file = new File(classOutputDir, ctClass.getSimpleName() + ".java"); final StringBuilder sb = new StringBuilder(); - sb.append("// Synthetic source — Javassist compile input for ") + sb.append("// Synthetic source - Javassist compile input for ") .append(ctClass.getSimpleName()).append("\n") .append("// Written when SW_DYNAMIC_CLASS_ENGINE_DEBUG is on; used by IDE\n") .append("// source-attach to render the bytecode without FernFlower.\n\n"); @@ -648,7 +588,10 @@ private void writeSourceFile(final CtClass ctClass, } sb.append(" ").append(executeBody.replace("\n", "\n ")).append("\n"); sb.append("}\n"); - try (java.io.FileWriter w = new java.io.FileWriter(file)) { + // UTF-8 explicitly, NOT the platform default: a FileWriter encodes in whatever charset + // the JVM happens to default to, while every reader of these files opens them as UTF-8. + try (Writer w = new OutputStreamWriter( + new FileOutputStream(file), StandardCharsets.UTF_8)) { w.write(sb.toString()); } catch (Exception e) { log.warn("Failed to write source file {}: {}", file, e.getMessage()); diff --git a/oap-server/analyzer/log-analyzer/src/test/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALSourceAttributionScriptTest.java b/oap-server/analyzer/log-analyzer/src/test/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALSourceAttributionScriptTest.java new file mode 100644 index 000000000000..8c4118165d6a --- /dev/null +++ b/oap-server/analyzer/log-analyzer/src/test/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALSourceAttributionScriptTest.java @@ -0,0 +1,204 @@ +/* + * 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.log.analyzer.v2.compiler; + +import java.io.BufferedInputStream; +import java.io.DataInputStream; +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import javassist.bytecode.ClassFile; +import javassist.bytecode.CodeAttribute; +import javassist.bytecode.LineNumberAttribute; +import javassist.bytecode.MethodInfo; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.yaml.snakeyaml.Yaml; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Drives the source-attribution contract from a dedicated, TEST-ONLY LAL script. + * + *

{@code source-attribution.yaml} is not a copy of any shipped config: the bundled LAL rules + * change with feature work, and pinning generated-file geometry to one of them would break for + * reasons unrelated to attribution. + * + *

Its three rules differ in GENERATED SHAPE — none, one, and two extractor blocks — because the + * shape is what any line number would depend on: each extractor becomes a private method emitted + * ahead of {@code execute()}, shifting everything after it. That variation is the point. A single + * simple rule would exercise only one of the two call sites that used to attach a line table, and + * would pass even if the other still emitted ordinals. + */ +class LALSourceAttributionScriptTest { + + private static final String SCRIPT = + "scripts/lal/test-lal/feature-cases/source-attribution.yaml"; + + private List dsls; + + @TempDir + File outputDir; + + @BeforeEach + @SuppressWarnings("unchecked") + void loadScript() throws Exception { + final Path path = Paths.get("src/test/resources").resolve(SCRIPT); + assertTrue(Files.isRegularFile(path), "dedicated LAL script missing: " + path); + + final Map doc = new Yaml().load( + new String(Files.readAllBytes(path), StandardCharsets.UTF_8)); + final List> rules = (List>) doc.get("rules"); + assertTrue(rules != null && rules.size() >= 3, + "the script should carry several differently-shaped rules"); + + dsls = new ArrayList<>(); + for (final Map rule : rules) { + dsls.add((String) rule.get("dsl")); + } + } + + @Test + void everyRuleInTheScriptCompilesAndItsSourceFileResolves() throws Exception { + final List generated = compileAll(); + assertEquals(dsls.size(), generated.size(), + "each rule should produce one main class"); + + for (final File classFile : generated) { + final String sourceFile = sourceFileOf(classFile); + assertEquals(classFile.getName().replace(".class", ".java"), sourceFile, + "SourceFile must name the class's own sidecar"); + assertTrue(new File(classFile.getParentFile(), sourceFile).isFile(), + "SourceFile names a file that was never written: " + sourceFile); + } + } + + @Test + void noGeneratedMethodClaimsALineItCannotSubstantiate() throws Exception { + // The regression this pins: line numbers used to be statement ORDINALS (1, 2, 3...). While + // SourceFile named a nonexistent file that was inert. Once SourceFile resolves, an ordinal + // resolves too — line 1 of the sidecar is its synthetic comment header, which an IDE would + // present as the frame's source. Absent beats confidently wrong. + for (final File classFile : compileAll()) { + final List table = lineNumberTableOf(classFile); + assertTrue(table.isEmpty(), + "expected no LineNumberTable for " + classFile.getName() + " but found " + + table.size() + " entries, first pointing at line " + + (table.isEmpty() ? "-" : String.valueOf(table.get(0)[1]))); + } + } + + @Test + void theShapesReallyDoDifferSoTheGeometryIsExercised() throws Exception { + // Guards the fixture itself. If all three rules compiled to the same shape, the test above + // would be checking one code path three times while appearing to cover three. + final List generated = compileAll(); + // File.listFiles() has no defined order, so comparing first against last would be + // comparing two arbitrary elements. What the fixture actually needs to guarantee is that + // the shapes are not all identical, which is an order-free property of the SET. + final Set methodCounts = new HashSet<>(); + for (final File classFile : generated) { + methodCounts.add(methodCountOf(classFile)); + } + assertTrue(methodCounts.size() > 1, + "the rules should generate differently-shaped classes, but every one produced the " + + "same method count: " + methodCounts); + } + + private List compileAll() throws Exception { + final File dir = new File(outputDir, "run-" + dsls.size()); + if (dir.isDirectory()) { + deleteRecursively(dir); + } + assertTrue(dir.mkdirs() || dir.isDirectory(), "could not create " + dir); + + for (int i = 0; i < dsls.size(); i++) { + final LALClassGenerator generator = new LALClassGenerator(); + generator.setClassOutputDir(dir); + generator.setYamlSource("source-attribution.yaml:" + (i + 1)); + generator.setClassNameHint("attribution_" + i); + generator.compile(dsls.get(i)); + } + final File[] classes = dir.listFiles( + (d, name) -> name.endsWith(".class") && !name.contains("$")); + final List out = new ArrayList<>(); + if (classes != null) { + for (final File c : classes) { + out.add(c); + } + } + return out; + } + + private static void deleteRecursively(final File dir) { + final File[] children = dir.listFiles(); + if (children != null) { + for (final File child : children) { + deleteRecursively(child); + } + } + dir.delete(); + } + + private static String sourceFileOf(final File classFile) throws Exception { + try (DataInputStream in = new DataInputStream( + new BufferedInputStream(Files.newInputStream(classFile.toPath())))) { + return new ClassFile(in).getSourceFile(); + } + } + + private static int methodCountOf(final File classFile) throws Exception { + try (DataInputStream in = new DataInputStream( + new BufferedInputStream(Files.newInputStream(classFile.toPath())))) { + return new ClassFile(in).getMethods().size(); + } + } + + private static List lineNumberTableOf(final File classFile) throws Exception { + final List out = new ArrayList<>(); + try (DataInputStream in = new DataInputStream( + new BufferedInputStream(Files.newInputStream(classFile.toPath())))) { + final ClassFile cf = new ClassFile(in); + for (final MethodInfo mi : cf.getMethods()) { + final CodeAttribute code = mi.getCodeAttribute(); + if (code == null) { + continue; + } + final LineNumberAttribute lna = + (LineNumberAttribute) code.getAttribute(LineNumberAttribute.tag); + if (lna == null) { + continue; + } + for (int i = 0; i < lna.tableLength(); i++) { + out.add(new int[]{lna.startPc(i), lna.lineNumber(i)}); + } + } + } + return out; + } +} diff --git a/oap-server/analyzer/log-analyzer/src/test/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALSourceFileResolvesTest.java b/oap-server/analyzer/log-analyzer/src/test/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALSourceFileResolvesTest.java new file mode 100644 index 000000000000..93a1d4972c17 --- /dev/null +++ b/oap-server/analyzer/log-analyzer/src/test/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALSourceFileResolvesTest.java @@ -0,0 +1,110 @@ +/* + * 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.log.analyzer.v2.compiler; + +import java.io.BufferedInputStream; +import java.io.DataInputStream; +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import javassist.bytecode.ClassFile; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * A generated class's {@code SourceFile} must name the {@code .java} actually written beside it. + * + *

LAL previously stamped {@code "(execution-basic.yaml:304)auto-layer-not-set.java"} — the rule's + * YAML provenance — while writing {@code execution_basic_L304_auto_layer_not_set.java}. Those never + * match, so IDE source-attach could not resolve a single LAL frame. It reads as correct precisely + * because the name looks informative; the failure only shows up when someone tries to open it. + * + *

Provenance belongs in the debug API, which reports the rule's YAML location per record. The + * bytecode attribute has exactly one job: name a file that exists. + */ +class LALSourceFileResolvesTest { + + @TempDir + File outputDir; + + private static final String DSL = + "filter {\n" + + " text {\n" + + " abortOnFailure false\n" + + " }\n" + + " sink {\n" + + " }\n" + + "}\n"; + + @Test + void theSourceFileAttributeNamesTheSidecarThatWasActuallyWritten() throws Exception { + final LALClassGenerator generator = new LALClassGenerator(); + generator.setClassOutputDir(outputDir); + generator.setYamlSource("execution-basic.yaml:304"); + generator.setClassNameHint("auto-layer-not-set"); + generator.compile(DSL); + + final File[] classes = outputDir.listFiles( + (dir, name) -> name.endsWith(".class") && !name.contains("$")); + assertNotNull(classes, "nothing written to " + outputDir); + assertTrue(classes.length > 0, "no .class written"); + + for (final File generated : classes) { + final String sourceFile; + try (DataInputStream in = new DataInputStream( + new BufferedInputStream(Files.newInputStream(generated.toPath())))) { + sourceFile = new ClassFile(in).getSourceFile(); + } + assertEquals(generated.getName().replace(".class", ".java"), sourceFile, + "SourceFile must name the class's own sidecar, not the rule's YAML provenance"); + assertTrue(new File(outputDir, sourceFile).isFile(), + "SourceFile names a file that was never written: " + sourceFile); + } + } + + @Test + void theSidecarIsUtf8RegardlessOfThePlatformDefaultCharset() throws Exception { + final LALClassGenerator generator = new LALClassGenerator(); + generator.setClassOutputDir(outputDir); + generator.setYamlSource("execution-basic.yaml:304"); + generator.setClassNameHint("auto-layer-not-set"); + generator.compile(DSL); + + final File[] sidecars = outputDir.listFiles((dir, name) -> name.endsWith(".java")); + assertNotNull(sidecars); + assertTrue(sidecars.length > 0, "no .java written"); + + for (final File sidecar : sidecars) { + // Decoding as UTF-8 must not throw. A FileWriter on a JVM defaulting to GBK / + // windows-1252 would have written bytes that are not valid UTF-8, which is exactly + // how the equivalent MAL sidecar broke CI with MalformedInputException. + final String content = + new String(Files.readAllBytes(sidecar.toPath()), StandardCharsets.UTF_8); + assertTrue(content.contains("Synthetic source"), "header missing in " + sidecar); + for (int i = 0; i < content.length(); i++) { + assertTrue(content.charAt(i) != '�', + "replacement char at " + i + " means the file is not valid UTF-8: " + sidecar); + } + } + } +} diff --git a/oap-server/analyzer/log-analyzer/src/test/resources/scripts/lal/test-lal/feature-cases/source-attribution.yaml b/oap-server/analyzer/log-analyzer/src/test/resources/scripts/lal/test-lal/feature-cases/source-attribution.yaml new file mode 100644 index 000000000000..350b014510ca --- /dev/null +++ b/oap-server/analyzer/log-analyzer/src/test/resources/scripts/lal/test-lal/feature-cases/source-attribution.yaml @@ -0,0 +1,70 @@ +# 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. + +# Dedicated to the source-attribution contract: SourceFile must name the .java actually written +# beside the .class, and no bytecode line may be attributed to a line that is not the statement +# it came from. +# +# TEST-ONLY on purpose. The bundled LAL configs change with feature work, and an assertion about +# generated-file geometry pinned to one of them would break for reasons unrelated to attribution. +# Nothing here is a copy of a shipped rule. +# +# The rules below deliberately differ in GENERATED SHAPE, because that shape is what the geometry +# depends on: how many private methods precede execute(), and therefore where any line would fall. +rules: + # Minimal: no extractor, so the fewest private methods. + - name: attribution-minimal + layer: GENERAL + dsl: | + filter { + text { + abortOnFailure false + } + sink { + } + } + + # One extractor: adds an _extractor() private method ahead of execute(). + - name: attribution-one-extractor + layer: GENERAL + dsl: | + filter { + json {} + extractor { + service parsed.service as String + endpoint parsed.endpoint as String + } + sink { + } + } + + # Two extractors plus a conditional: the most private methods, so the largest geometry shift. + # If a line number were ever emitted from a fixed offset, this rule is where it would be wrong. + - name: attribution-multi-block + layer: GENERAL + dsl: | + filter { + json {} + extractor { + service parsed.service as String + } + if (tag("LOG_KIND") == "SLOW_SQL") { + extractor { + endpoint parsed.endpoint as String + } + } + sink { + } + } diff --git a/oap-server/analyzer/meter-analyzer/CLAUDE.md b/oap-server/analyzer/meter-analyzer/CLAUDE.md index fc79eabb8e39..56034fd49f28 100644 --- a/oap-server/analyzer/meter-analyzer/CLAUDE.md +++ b/oap-server/analyzer/meter-analyzer/CLAUDE.md @@ -248,8 +248,24 @@ When `SW_DYNAMIC_CLASS_ENGINE_DEBUG=true` environment variable is set, generated *.class — Main MalExpression class per expression *.java — Javassist compile input (synthetic; for IDE source-attach) *$_tag.class — Companion class per closure (one per tag/forEach/instance/decorate call) + *$_tag.java — Companion compile input, one per companion class ``` +**One rule, N class files.** Javassist cannot emit lambdas or anonymous inner classes, so every +closure becomes its own class, and the JVM allows exactly one `SourceFile` per class. A rule with +two closures produces three class files, each needing its own file name and its own line numbering. +That is why the YAML line and the generated-class line cannot be one number — their cardinality +differs (one YAML anchor to N generated files; a file-level `filter:` maps the other way, one class +shared by every rule in the document). `MalSourceRef` is the shared structure holding both, one +instance per generated file. + +Companions carry a **single** `LineNumberTable` entry pointing at their SAM signature, not a +per-statement table. The per-statement scan in `addLineNumberTable` marks boundaries at stores to a +result slot; a closure body stores nothing there, so on the shipped rule set 96% of companions +would collapse to one entry anyway while the `forEach` ones over-count inside `if/else` chains. +`MALClosureCodegen.COMPANION_SAM_LINE_IN_CLASS` counts the sidecar's envelope — change +`wrapCompanionSource` and that constant must change with it (`MalCompanionSourceTest` fails if not). + The `.java` sidecar exists so IDE source-attach renders the actual codegen input directly without relying on FernFlower / a decompiler. Javassist-emitted bytecode often confuses decompilers (no `goto` consolidation, slot reuse with mixed types, debug-injected `if (gate.isGateOn()) { ... }` chains), and FernFlower frequently bails to "compiled code" stubs. The source-attach path always works and shows the EXACT code Javassist compiled — gate fields, probe call sites, closure dispatch, the lot. When `SW_DSL_DEBUGGING_INJECTION_ENABLED=true` is also set, the codegen emits the per-rule `GateHolder debug` field plus `MALDebug.captureXxx(...)` probe sites at every chain stage. Both `.class` and `.java` reflect the with-debug shape. The two env vars are independent: `SW_DYNAMIC_CLASS_ENGINE_DEBUG` controls disk dump; `SW_DSL_DEBUGGING_INJECTION_ENABLED` controls codegen branch. diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MalYamlLineIndex.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MalYamlLineIndex.java new file mode 100644 index 000000000000..52bda45fd1e8 --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MalYamlLineIndex.java @@ -0,0 +1,192 @@ +/* + * 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.meter.analyzer.v2; + +import java.io.StringReader; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import lombok.Getter; +import lombok.extern.slf4j.Slf4j; +import org.yaml.snakeyaml.LoaderOptions; +import org.yaml.snakeyaml.Yaml; +import org.yaml.snakeyaml.nodes.MappingNode; +import org.yaml.snakeyaml.nodes.Node; +import org.yaml.snakeyaml.nodes.NodeTuple; +import org.yaml.snakeyaml.nodes.ScalarNode; +import org.yaml.snakeyaml.nodes.SequenceNode; + +/** + * Resolves the 1-based YAML line of each source fragment in a MAL rule file. + * + *

SnakeYAML's bean binding ({@code Yaml.loadAs}) discards positional {@code Mark} data, so the + * rule objects the loaders hand to {@link MetricConvert} carry no idea where they came from. This + * class runs a second {@code compose} pass over the SAME text and reads the marks off the node + * tree, which is why callers must give it the exact bytes they bound from — re-reading the file + * could pick up a different revision. + * + *

Four anchors are resolved, because a compiled MAL expression is a splice of up to three + * separate source locations plus a separately-compiled filter: + *

    + *
  • {@code filter} / {@code expPrefix} / {@code expSuffix} — file-level, one line each.
    • + *
  • per {@code metricsRules} entry — the entry's own line (the {@code - name:} anchor, used + * for the generated class label) and its {@code exp:} line.
    • + *
+ * + *

Every accessor returns {@code 0} for "unknown", which every consumer treats as "omit the line" + * rather than "line zero". Resolution is best-effort by design: a malformed or unusual document + * degrades to zeros instead of failing the rule load, because a missing line number must never + * stop a rule from compiling. + */ +@Slf4j +public final class MalYamlLineIndex { + + /** Rules key in the standard MAL rule file. Zabbix uses {@code metrics} instead. */ + public static final String DEFAULT_RULES_KEY = "metricsRules"; + + private static final MalYamlLineIndex EMPTY = + new MalYamlLineIndex(0, 0, 0, Collections.emptyList()); + + @Getter + private final int filterLine; + @Getter + private final int expPrefixLine; + @Getter + private final int expSuffixLine; + private final List rules; + + private MalYamlLineIndex(final int filterLine, final int expPrefixLine, + final int expSuffixLine, final List rules) { + this.filterLine = filterLine; + this.expPrefixLine = expPrefixLine; + this.expSuffixLine = expSuffixLine; + this.rules = rules; + } + + /** Per-{@code metricsRules}-entry anchors. */ + @Getter + public static final class RuleLines { + /** Line of the entry itself — the {@code - name:} anchor. */ + private final int entryLine; + /** Line of the entry's {@code exp:} key. */ + private final int expLine; + + RuleLines(final int entryLine, final int expLine) { + this.entryLine = entryLine; + this.expLine = expLine; + } + } + + /** All-zero index, for callers with no YAML text to inspect. */ + public static MalYamlLineIndex empty() { + return EMPTY; + } + + public static MalYamlLineIndex index(final String yamlContent) { + return index(yamlContent, DEFAULT_RULES_KEY); + } + + /** + * @param yamlContent the exact text the rule object was bound from + * @param rulesKey key holding the rules sequence — {@link #DEFAULT_RULES_KEY} for standard + * MAL files, {@code metrics} for zabbix + * @return resolved anchors, or an all-zero index when the document can't be inspected + */ + public static MalYamlLineIndex index(final String yamlContent, final String rulesKey) { + if (yamlContent == null || yamlContent.isEmpty()) { + return EMPTY; + } + try (StringReader reader = new StringReader(yamlContent)) { + final Node root = new Yaml(new LoaderOptions()).compose(reader); + if (!(root instanceof MappingNode)) { + return EMPTY; + } + int filter = 0; + int prefix = 0; + int suffix = 0; + List ruleLines = Collections.emptyList(); + for (final NodeTuple tuple : ((MappingNode) root).getValue()) { + final String key = scalarKey(tuple.getKeyNode()); + if (key == null) { + continue; + } + switch (key) { + case "filter": + filter = lineOf(tuple.getKeyNode()); + break; + case "expPrefix": + prefix = lineOf(tuple.getKeyNode()); + break; + case "expSuffix": + suffix = lineOf(tuple.getKeyNode()); + break; + default: + if (key.equals(rulesKey)) { + ruleLines = indexRules(tuple.getValueNode()); + } + break; + } + } + return new MalYamlLineIndex(filter, prefix, suffix, ruleLines); + } catch (final RuntimeException e) { + // Malformed YAML is the loader's problem to report, not ours — it will surface a far + // better message than we could. Degrade to "no lines known". + log.debug("MAL YAML line index unavailable: {}", e.getMessage()); + return EMPTY; + } + } + + /** Anchors for the rules entry at {@code index}, or an all-zero entry when out of range. */ + public RuleLines rule(final int index) { + if (index < 0 || index >= rules.size()) { + return new RuleLines(0, 0); + } + return rules.get(index); + } + + private static List indexRules(final Node rulesNode) { + if (!(rulesNode instanceof SequenceNode)) { + return Collections.emptyList(); + } + final List items = ((SequenceNode) rulesNode).getValue(); + final List out = new ArrayList<>(items.size()); + for (final Node item : items) { + int expLine = 0; + if (item instanceof MappingNode) { + for (final NodeTuple tuple : ((MappingNode) item).getValue()) { + if ("exp".equals(scalarKey(tuple.getKeyNode()))) { + expLine = lineOf(tuple.getKeyNode()); + break; + } + } + } + out.add(new RuleLines(lineOf(item), expLine)); + } + return out; + } + + private static String scalarKey(final Node node) { + return node instanceof ScalarNode ? ((ScalarNode) node).getValue() : null; + } + + /** SnakeYAML marks are 0-based; every consumer of this class speaks 1-based lines. */ + private static int lineOf(final Node node) { + return node == null || node.getStartMark() == null ? 0 : node.getStartMark().getLine() + 1; + } +} diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MetricConvert.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MetricConvert.java index 4353339755ad..cde9cc202776 100644 --- a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MetricConvert.java +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MetricConvert.java @@ -34,6 +34,7 @@ import lombok.Getter; import lombok.extern.slf4j.Slf4j; import org.apache.skywalking.oap.meter.analyzer.v2.compiler.MALScriptParser; +import org.apache.skywalking.oap.meter.analyzer.v2.compiler.MalSourceRef; import org.apache.skywalking.oap.meter.analyzer.v2.dsl.FilterExpression; import org.apache.skywalking.oap.meter.analyzer.v2.dsl.SampleFamily; import org.apache.skywalking.oap.server.core.analysis.meter.MeterSystem; @@ -132,8 +133,12 @@ public MetricConvert(final MetricRuleConfig rule, final MeterSystem service, final List prepared = IntStream.range(0, rules.size()).mapToObj( i -> { final MetricRuleConfig.RuleConfig r = rules.get(i); - final String yamlSource = sourceName != null - ? sourceName + ".yaml:" + i : null; + // The rule's REAL line, not its position in the list. The two only ever agreed by + // accident, which is why a stack trace used to point at the top of the file. + // A line is always expected here; -1 marks a resolution failure so it stays + // visible in the artifact rather than silently degrading. + final String yamlSource = sourceName == null ? null + : MalSourceRef.ofRule(sourceName + ".yaml", r.getLineNo()).describeYaml(); final Analyzer analyzer = prepareAnalyzer( formatMetricName(rule, r.getName()), filter, @@ -289,8 +294,9 @@ private static FilterExpression buildFilter(final MetricRuleConfig rule, return null; } final String sourceName = rule.getSourceName(); - final String yamlSource = sourceName != null - ? sourceName + ".yaml" : null; + // The filter has its own line, distinct from any rule's — it is file-level. + final String yamlSource = sourceName == null ? null + : MalSourceRef.ofRule(sourceName + ".yaml", rule.getFilterLine()).describeYaml(); return new FilterExpression(filterText, "filter", yamlSource, pool, targetClassLoader); } diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MetricRuleConfig.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MetricRuleConfig.java index 42f55eebebd3..b95fd32b37b6 100644 --- a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MetricRuleConfig.java +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/MetricRuleConfig.java @@ -58,6 +58,31 @@ default String getSourceName() { return null; } + /** + * 1-based line of the file-level {@code filter:} key in the source YAML, or {@code 0} when + * unknown. A compiled MAL expression splices up to three separate source locations together, + * so each needs its own anchor; see {@link MalYamlLineIndex}. + * + * @return the filter's line, or {@code 0} + */ + default int getFilterLine() { + return 0; + } + + /** + * @return 1-based line of the file-level {@code expPrefix:} key, or {@code 0} when unknown + */ + default int getExpPrefixLine() { + return 0; + } + + /** + * @return 1-based line of the file-level {@code expSuffix:} key, or {@code 0} when unknown + */ + default int getExpSuffixLine() { + return 0; + } + interface RuleConfig { /** * Get definition metrics name @@ -68,5 +93,22 @@ interface RuleConfig { * Build metrics MAL */ String getExp(); + + /** + * 1-based line of this rule's entry (its {@code - name:} anchor) in the source YAML, or + * {@code 0} when unknown. Used as the provenance label in the generated class name. + * + * @return the entry's line, or {@code 0} + */ + default int getLineNo() { + return 0; + } + + /** + * @return 1-based line of this rule's {@code exp:} key, or {@code 0} when unknown + */ + default int getExpLine() { + return 0; + } } } diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALBytecodeHelper.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALBytecodeHelper.java index 7f7322d5c60c..49ba3d6d53d6 100644 --- a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALBytecodeHelper.java +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALBytecodeHelper.java @@ -20,6 +20,9 @@ import java.io.DataOutputStream; import java.io.File; import java.io.FileOutputStream; +import java.nio.charset.StandardCharsets; +import java.io.Writer; +import java.io.OutputStreamWriter; import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; @@ -27,6 +30,10 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import javassist.CtClass; +import javassist.CtMethod; +import javassist.bytecode.AttributeInfo; +import javassist.bytecode.CodeAttribute; +import javassist.bytecode.MethodInfo; import lombok.extern.slf4j.Slf4j; /** @@ -54,7 +61,8 @@ final class MALBytecodeHelper { private File classOutputDir; private String classNameHint; - private String yamlSource; + /** Rule anchor, parsed once on the way in so nothing downstream re-splits it. */ + private MalSourceRef ruleAnchor = MalSourceRef.ofRule(null, MalSourceRef.UNRESOLVED); /** * When true, each apply gets its own per-file classloader, so generated class names are * scoped to that loader and don't need the process-wide {@link #USED_CLASS_NAMES} dedup. @@ -76,7 +84,7 @@ String getClassNameHint() { } void setYamlSource(final String yamlSource) { - this.yamlSource = yamlSource; + this.ruleAnchor = MalSourceRef.parse(yamlSource); } void setPerFileClassLoader(final boolean perFileClassLoader) { @@ -101,29 +109,40 @@ String makeClassName(final String defaultPrefix) { private String buildHintedName() { final String hint = MALCodegenHelper.sanitizeName(classNameHint); - if (yamlSource == null) { + if (ruleAnchor.getYamlFile() == null) { return hint; } - String yamlBase = yamlSource; - String lineNo = null; - final int colonIdx = yamlSource.lastIndexOf(':'); - if (colonIdx > 0) { - yamlBase = yamlSource.substring(0, colonIdx); - lineNo = yamlSource.substring(colonIdx + 1); - } + String yamlBase = ruleAnchor.getYamlFile(); final int dotIdx = yamlBase.lastIndexOf('.'); if (dotIdx > 0) { yamlBase = yamlBase.substring(0, dotIdx); } final StringBuilder sb = new StringBuilder(); sb.append(MALCodegenHelper.sanitizeName(yamlBase)); - if (lineNo != null) { - sb.append("_L").append(lineNo); - } + // The unresolved marker is -1, which is not legal in a Java identifier; render it as + // "unknown" so the failure stays visible in the class name instead of vanishing. + sb.append("_L").append(MalSourceRef.toIdentifierSegment(ruleAnchor.getYamlLine())); sb.append('_').append(hint); return sb.toString(); } + /** + * Coordinates of one generated class: the rule anchor this helper was configured with, paired + * with that class's own file name and line. + * + *

No parsing happens here: {@link #setYamlSource} already resolved the anchor through + * {@link MalSourceRef#parse}, which is the single place the {@code "vm.yaml:38"} wire form is + * split. Two call sites each re-splitting it with their own {@code lastIndexOf(':')} is what + * let the file and the line drift apart in the first place. + * + * @param ctClass the generated class + * @param lineInClass 1-based line within that class's {@code .java} + * @return the paired coordinates for this one generated file + */ + MalSourceRef sourceRefOf(final CtClass ctClass, final int lineInClass) { + return ruleAnchor.inGeneratedClass(ctClass.getSimpleName(), lineInClass); + } + private String dedupClassName(final String base) { // Runtime-rule hot-update gives every apply its own RuleClassLoader — same class name // across applies lands in different loader namespaces. Skip the process-wide dedup set @@ -146,15 +165,21 @@ private String dedupClassName(final String base) { // ==================== Debug output ==================== /** - * Builds the SourceFile name for a generated class. - * Example: {@code "(vm.yaml:25)cpu_total.java"} + * Builds the {@code SourceFile} name for a generated class. + * + *

This MUST equal the file {@link #writeSourceFile} actually writes — that is the entire + * contract an IDE relies on to resolve a stack frame to that source. It previously returned + * {@code "(vm.yaml:25)cpu_total.java"} while the file on disk was named after the class + * (e.g. {@code vm_L25_cpu_total.java}), so source-attach could never resolve, independent of + * whether the embedded line was correct. + * + *

The YAML provenance is NOT repeated here. It already lives in the class name (and hence + * in this file name), and the per-statement YAML lines belong to the operator coordinate + * carried on {@link MalSourceRef}, not to a per-file bytecode attribute that can only hold + * one value for a rule spliced from three source locations. */ - String formatSourceFileName(final String metricName) { - final String classFile = metricName + ".java"; - if (yamlSource != null) { - return "(" + yamlSource + ")" + classFile; - } - return classFile; + static String sourceFileNameOf(final CtClass ctClass) { + return ctClass.getSimpleName() + ".java"; } /** @@ -215,8 +240,15 @@ void writeSourceFile(final CtClass ctClass, final String javaSource) { } final File file = new File( classOutputDir, ctClass.getSimpleName() + ".java"); - try (java.io.FileWriter w = new java.io.FileWriter(file)) { - w.write("// Synthetic source — Javassist compile input for "); + // UTF-8 explicitly, NOT the platform default. A FileWriter encodes in whatever charset the + // JVM happens to default to, which varies by machine and by CI runner, while every reader + // of these files -- IDE source-attach and the tests -- opens them as UTF-8. The generated + // source must decode the same way everywhere, so the encoding is pinned at the writer + // rather than left to depend on where the build ran. (project.build.sourceEncoding governs + // compilation only; it does not reach a FileWriter opened at runtime.) + try (Writer w = new OutputStreamWriter( + new FileOutputStream(file), StandardCharsets.UTF_8)) { + w.write("// Synthetic source - Javassist compile input for "); w.write(ctClass.getSimpleName()); w.write("\n// Written when SW_DYNAMIC_CLASS_ENGINE_DEBUG is on; used by IDE\n"); w.write("// source-attach to render the bytecode without FernFlower.\n\n"); @@ -232,12 +264,28 @@ void writeSourceFile(final CtClass ctClass, final String javaSource) { // ==================== Bytecode attributes ==================== /** - * Adds a {@code LineNumberTable} attribute to the method. - * Scans bytecode for store instructions to local variable slots - * >= {@code firstResultSlot}, assigning sequential line numbers. + * Adds a {@code LineNumberTable} mapping each bytecode boundary to its line in the + * {@code .java} source written for the generated class. + * + *

Boundaries are the first instruction plus the instruction after every store to a local + * slot {@code >= firstResultSlot}, which for the variable-per-expression codegen is one per + * emitted statement. {@code boundaryLines} must list the corresponding method-relative source + * lines in the same order — {@link MalGeneratedSourceLines} derives them from the same + * generated text, so the two agree by construction. + * + *

This previously emitted {@code 1,2,3…} — statement ordinals that matched neither the generated + * class source nor the rule YAML, so no IDE could resolve a frame. + * + * @param method the generated method + * @param firstResultSlot lowest local slot that holds a statement result + * @param statementLines 1-based lines WITHIN THE GENERATED METHOD, in boundary order + * @param methodSignatureLineInClass line the method signature occupies in the generated + * class source, used to convert method-relative to class-relative */ void addLineNumberTable(final javassist.CtMethod method, - final int firstResultSlot) { + final int firstResultSlot, + final java.util.List statementLines, + final int methodSignatureLineInClass) { try { final javassist.bytecode.MethodInfo mi = method.getMethodInfo(); final javassist.bytecode.CodeAttribute code = @@ -246,14 +294,15 @@ void addLineNumberTable(final javassist.CtMethod method, return; } final List entries = new ArrayList<>(); - int line = 1; + int boundary = 0; boolean nextIsNewLine = true; final javassist.bytecode.CodeIterator ci = code.iterator(); while (ci.hasNext()) { final int pc = ci.next(); if (nextIsNewLine) { - entries.add(new int[]{pc, line++}); + entries.add(new int[]{pc, lineInGeneratedClass(statementLines, boundary, methodSignatureLineInClass)}); + boundary++; nextIsNewLine = false; } final int op = ci.byteAt(pc) & 0xFF; @@ -271,6 +320,30 @@ void addLineNumberTable(final javassist.CtMethod method, if (entries.isEmpty()) { return; } + if (statementLines == null || statementLines.isEmpty() + || entries.size() != statementLines.size()) { + // Emit NOTHING rather than a table we already know is wrong. A desync means a + // codegen change altered the statement shape without the scanner learning about + // it, so the surviving entries are shifted -- plausible, and wrong. Note also + // that line_number is an unsigned u2, so an UNRESOLVED (-1) would serialize as + // 65535 and read as a real line, not as "unknown". Same policy as companion + // classes: no attribution beats false attribution, and an absent table makes the + // JVM report an unknown line honestly. + if (statementLines != null && !statementLines.isEmpty()) { + log.warn("MAL LineNumberTable omitted for {}: {} bytecode boundaries vs {} " + + "source lines. Frames will report an unknown line.", + method.getName(), entries.size(), statementLines.size()); + } + return; + } + for (final int[] entry : entries) { + if (entry[1] <= 0) { + // An unresolved line cannot be represented: u2 would turn -1 into 65535. + log.warn("MAL LineNumberTable omitted for {}: an entry had no resolvable " + + "line.", method.getName()); + return; + } + } final javassist.bytecode.ConstPool cp = mi.getConstPool(); final byte[] info = new byte[2 + entries.size() * 4]; info[0] = (byte) (entries.size() >> 8); @@ -290,6 +363,69 @@ void addLineNumberTable(final javassist.CtMethod method, } } + /** + * Adds a single-entry {@code LineNumberTable} pointing the whole method at the line its + * signature occupies in the generated {@code .java}. + * + *

For a companion the per-statement scan in {@link #addLineNumberTable} is unusable: it + * marks boundaries at stores to a result slot, and a closure body stores nothing there. On the + * shipped rule set that means 96% of companions would collapse to one entry regardless, while + * the {@code forEach} companions over-count — 8 boundaries for 5 statements, because the scan + * also fires inside an {@code if/else-if/else} chain. Either way the per-statement table would + * be wrong in a way that reads as right. + * + *

One correct entry is therefore the honest maximum here, and it is worth having: a frame + * inside a closure resolves to the companion's own source file at its declaration, which is + * what an IDE needs in order to open it. Pointing at the SIGNATURE rather than the first + * statement is what keeps that true — the signature is the one line whose position does not + * depend on the body's shape. + * + * @param method the generated SAM method + * @param lineInClass line the method signature occupies in the generated class source + */ + void addMethodEntryLineNumber(final CtMethod method, final int lineInClass) { + if (lineInClass <= 0) { + // u2 cannot represent UNRESOLVED: -1 would serialize as 65535 and read as a real line. + return; + } + try { + final MethodInfo mi = method.getMethodInfo(); + final CodeAttribute code = mi.getCodeAttribute(); + if (code == null) { + return; + } + final byte[] info = new byte[]{ + 0, 1, + 0, 0, + (byte) (lineInClass >> 8), (byte) lineInClass, + }; + code.getAttributes().add( + new AttributeInfo(mi.getConstPool(), "LineNumberTable", info)); + } catch (Exception e) { + log.warn("Failed to add companion LineNumberTable: {}", e.getMessage()); + } + } + + /** + * Sidecar line for boundary {@code i}, or {@link MalSourceRef#UNRESOLVED} when the scanner + * produced no line for it. {@code -1} is deliberate: a bogus-but-plausible line is worse than + * an obviously absent one, and the JVM tolerates it in a {@code LineNumberTable}. + */ + private static int lineInGeneratedClass(final java.util.List statementLines, + final int index, + final int methodSignatureLineInClass) { + if (statementLines == null || index >= statementLines.size()) { + return MalSourceRef.UNRESOLVED; + } + final Integer lineInMethod = statementLines.get(index); + if (lineInMethod == null || lineInMethod <= 0) { + return MalSourceRef.UNRESOLVED; + } + // Method-relative line 1 IS the signature line, so the class-relative line counts from + // there rather than adding a bare offset. + return methodSignatureLineInClass + lineInMethod - 1; + } + /** * Adds a {@code LocalVariableTable} attribute for debug info. */ diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALClassGenerator.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALClassGenerator.java index 5fdb8c273a5a..c3657645214d 100644 --- a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALClassGenerator.java +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALClassGenerator.java @@ -200,11 +200,12 @@ public MalFilter compileFilter(final String filterExpression) throws Exception { ctClass.addMethod(testMethod); bytecodeHelper.addLocalVariableTable(testMethod, className, new String[][]{{paramName, "Ljava/util/Map;"}}); - bytecodeHelper.addLineNumberTable(testMethod, 2); + // wrapMalFilterSource has no leading blank before the body, so the signature sits one + // line higher than the expression class's: file line 9. + bytecodeHelper.addLineNumberTable(testMethod, 2, + MalGeneratedSourceLines.statementLinesOf(filterBody), 9); MALBytecodeHelper.setSourceFile(ctClass, - bytecodeHelper.formatSourceFileName( - bytecodeHelper.getClassNameHint() != null - ? bytecodeHelper.getClassNameHint() : "filter")); + MALBytecodeHelper.sourceFileNameOf(ctClass)); bytecodeHelper.writeClassFile(ctClass); bytecodeHelper.writeSourceFile(ctClass, wrapMalFilterSource(ctClass, filterBody)); @@ -344,7 +345,15 @@ public MalExpression compileFromModel(final String metricName, ctClass.addMethod(runMethod); bytecodeHelper.addRunLocalVariableTable( runMethod, className, exprCodegen.getDeclaredVars()); - bytecodeHelper.addLineNumberTable(runMethod, 1); + // Sidecar geometry: writeSourceFile adds a 4-line preamble, then wrapMalExpressionSource + // emits package(2) + class decl(2) + one line per closure field + 2 injection members when + // enabled + 1 blank, so the run() signature lands at line 10 + C + I of the generated + // class source. The helper converts method-relative lines from there. + final int runMethodSignatureLineInClass = 10 + closureFieldNames.size() + + (org.apache.skywalking.oap.server.core.dsldebug.DSLDebugCodegenSwitch + .isInjectionEnabled() ? 2 : 0); + bytecodeHelper.addLineNumberTable(runMethod, 1, + MalGeneratedSourceLines.statementLinesOf(runBody), runMethodSignatureLineInClass); final javassist.CtMethod metaMethod = CtNewMethod.make(metadataBody, ctClass); @@ -357,7 +366,7 @@ public MalExpression compileFromModel(final String metricName, {"_pct", "[I"} }); MALBytecodeHelper.setSourceFile(ctClass, - bytecodeHelper.formatSourceFileName(metricName)); + MALBytecodeHelper.sourceFileNameOf(ctClass)); // 6. Load companions, then main class for (final CtClass companion : companionClasses) { diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALClosureCodegen.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALClosureCodegen.java index e12a6b7fdcf3..b720217e7549 100644 --- a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALClosureCodegen.java +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALClosureCodegen.java @@ -34,6 +34,16 @@ @Slf4j final class MALClosureCodegen { + /** + * Line the companion's SAM signature occupies in its written {@code .java}. + * + *

{@code writeSourceFile} emits a 4-line preamble, then {@link #wrapCompanionSource} adds + * {@code package}(1) + blank(1) + class declaration(1) + blank(1), putting the signature on + * line 9. Unlike the main expression class this is a constant, not {@code 10 + fields + + * injection}: a companion declares no closure fields and no injected {@code GateHolder}. + */ + private static final int COMPANION_SAM_LINE_IN_CLASS = 9; + private final ClassPool classPool; private final MALBytecodeHelper bytecodeHelper; @@ -626,7 +636,14 @@ CtClass makeCompanionClass(final CtClass mainClass, final javassist.CtMethod m = CtNewMethod.make(methodBody, companion); companion.addMethod(m); addCompanionLocalVariableTable(m, info); - bytecodeHelper.addLineNumberTable(m, firstResultSlot(info)); + // A companion is its own class file, so it carries its own generated-source coordinates: + // its own .java, and a line into THAT file. Javassist's ClassFile constructor already + // names the file in the SourceFile attribute (simple name + ".java"); writing the file is + // what stops that name from dangling, which it did for every companion until now. + // One entry rather than a per-statement table -- see addMethodEntryLineNumber. + final MalSourceRef ref = bytecodeHelper.sourceRefOf(companion, COMPANION_SAM_LINE_IN_CLASS); + bytecodeHelper.writeSourceFile(companion, wrapCompanionSource(companion, info, methodBody)); + bytecodeHelper.addMethodEntryLineNumber(m, ref.getGeneratedLine()); return companion; } @@ -732,14 +749,29 @@ private void addCompanionLocalVariableTable(final javassist.CtMethod m, } } - private int firstResultSlot(final ClosureInfo info) { - if (MALCodegenHelper.FOR_EACH_FUNCTION_TYPE.equals(info.interfaceType)) { - return 3; // slot 0=this, 1=element, 2=tags, 3+=locals - } else if (MALCodegenHelper.DECORATE_FUNCTION_TYPE.equals(info.interfaceType)) { - return 3; // slot 0=this, 1=_arg, 2=paramName, 3+=locals - } else { - return 3; // slot 0=this, 1=_raw, 2=paramName, 3+=locals - } + /** + * The companion's {@code .java} sidecar: the same envelope handed to Javassist, so + * source-attach renders exactly the code that was compiled. + * + *

The line geometry here is load-bearing — {@link #COMPANION_SAM_LINE_IN_CLASS} counts + * these lines, so changing the envelope without changing that constant silently mis-attributes + * every closure frame. + * + * @param companion the companion class being generated + * @param info the closure it implements, for the {@code implements} clause + * @param methodBody the SAM method source fed to Javassist + * @return the full source of the sidecar file + */ + private static String wrapCompanionSource(final CtClass companion, + final ClosureInfo info, + final String methodBody) { + final StringBuilder sb = new StringBuilder(); + sb.append("package ").append(companion.getPackageName()).append(";\n\n"); + sb.append("public class ").append(companion.getSimpleName()) + .append(" implements ").append(info.interfaceType).append(" {\n\n"); + sb.append(" ").append(methodBody.replace("\n", "\n ")).append("\n"); + sb.append("}\n"); + return sb.toString(); } void generateClosureCondition(final StringBuilder sb, diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalGeneratedSourceLines.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalGeneratedSourceLines.java new file mode 100644 index 000000000000..e32d0d1fe84a --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalGeneratedSourceLines.java @@ -0,0 +1,89 @@ +/* + * 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.meter.analyzer.v2.compiler; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +/** + * Finds the method-body lines that {@code addLineNumberTable}'s bytecode scan will mark as + * boundaries, so the two agree by construction. + * + *

The alternative was recording a line at each of the dozen places the codegen appends a + * statement. That drifts: a new emit site added later silently desynchronises the list from the + * bytecode, and because the numbers still look plausible the damage is invisible. Deriving both + * from the same generated text keeps them in lockstep — the source we scan is source we wrote, and + * it is fully deterministic. + * + *

What counts as a boundary. {@code addLineNumberTable} emits one entry for the first + * instruction, then one after every store to a result slot. So the boundary lines are, in order: + * every statement that assigns a result variable, followed by the terminating {@code return}. Any + * line that emits no such store — notably the {@code MALDebug.captureXxx(...)} probe calls, which + * are present only when debug injection is enabled — is deliberately skipped. That is exactly why + * a fixed "statement N is at line N+1" offset cannot be used: enabling injection doubles the + * source lines without adding boundaries. + */ +final class MalGeneratedSourceLines { + + private MalGeneratedSourceLines() { + } + + /** + * @param generatedMethodBody the generated method source, starting at its signature line + * @return 1-based lines WITHIN THAT METHOD for each statement the bytecode scan will mark, + * in order + */ + static List statementLinesOf(final String generatedMethodBody) { + if (generatedMethodBody == null || generatedMethodBody.isEmpty()) { + return Collections.emptyList(); + } + final String[] lines = generatedMethodBody.split("\n", -1); + final List out = new ArrayList<>(); + for (int i = 0; i < lines.length; i++) { + final String trimmed = lines[i].trim(); + if (isResultAssignment(trimmed) || trimmed.startsWith("return ")) { + // 1-based, and the array index already excludes nothing — line 1 is the signature. + out.add(i + 1); + } + } + return out; + } + + /** + * A result assignment is {@code _var = ...;} or its declaring form {@code SampleFamily _var = + * ...;}. Both compile to a store into the result slot; a probe call or a bare expression + * statement does not. + */ + private static boolean isResultAssignment(final String trimmed) { + if (!trimmed.endsWith(";") || trimmed.startsWith("return ")) { + return false; + } + final int eq = trimmed.indexOf(" = "); + if (eq <= 0) { + return false; + } + // The assignment target is the last token before " = ", and every result variable the + // codegen mints is prefixed with '_' (MALCodegenHelper's variable-per-expression scheme). + final String lhs = trimmed.substring(0, eq); + final int lastSpace = lhs.lastIndexOf(' '); + final String target = lastSpace < 0 ? lhs : lhs.substring(lastSpace + 1); + return target.startsWith("_"); + } +} diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalSourceRef.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalSourceRef.java new file mode 100644 index 000000000000..d62841df5d27 --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalSourceRef.java @@ -0,0 +1,206 @@ +/* + * 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.meter.analyzer.v2.compiler; + +import lombok.Getter; + +/** + * Where one generated class came from, in both coordinate spaces at once. + * + *

Compiling a MAL rule produces MORE THAN ONE class file. Javassist cannot emit lambdas or + * anonymous inner classes, so every closure becomes its own class, and the JVM allows exactly one + * {@code SourceFile} per class: + * + * 

+ *   metricsRules:
+ *     - name: cpu_total                                   vm.yaml:38   <- ONE operator anchor
+ *       exp: node_cpu.tag({...}).forEach({...}).sum([...])
+ *            |
+ *            +-- vm_L38_cpu_total.java            run()   <- THREE generated files,
+ *            +-- vm_L38_cpu_total$_tag.java       apply()     each with its own
+ *            +-- vm_L38_cpu_total$_forEach.java   accept()    name and line numbering
+ *
+ * + *

That is why the two spaces cannot collapse into a single number: their CARDINALITY differs. + * One YAML line maps to N generated files, and a file-level {@code filter:} maps the other way — + * one line, one class, shared by every rule in the document. A lone {@code int line} field cannot + * express either direction, which is how the {@code LineNumberTable} once ended up holding a + * statement ordinal that matched no file at all. + * + *

One instance describes ONE generated file. The class is shared by every kind of generated + * artifact — main expression class, filter class, closure companion — because they differ only in + * their values, not in their shape. + * + *

    + *
  • {@link #getYamlFile() yamlFile} / {@link #getYamlLine() yamlLine} — the OPERATOR + * coordinate, fixed per rule. What the dsl-debugging API reports so a captured sample names + * an editable location. Every generated file of one rule repeats the same pair; it is the + * join key tying them back together.
    • + *
  • {@link #getGeneratedClass() generatedClass} / {@link #getGeneratedLine() + * generatedLine} — the MACHINE coordinate, unique per generated file. What + * {@code SourceFile} and {@code LineNumberTable} carry so a stack frame or an IDE resolves + * to real source.
    • + *
+ * + *

Unresolved lines. A line is normally always available — rules are bound from YAML + * snakeyaml can re-compose, and generated lines are counted in buffers we build ourselves. + * {@link #UNRESOLVED} is a defensive fallback, deliberately {@code -1} rather than {@code 0}: a + * silently missing line hides the failure, whereas {@code -1} stays visible and greppable. It is + * never written into a {@code LineNumberTable}, where the unsigned {@code u2} encoding would turn + * it into 65535 and make it read as a real line. + */ +@Getter +public final class MalSourceRef { + + /** Marker for "a line was expected here but could not be resolved". */ + public static final int UNRESOLVED = -1; + + private final String yamlFile; + private final int yamlLine; + private final String generatedClass; + private final int generatedLine; + + private MalSourceRef(final String yamlFile, + final int yamlLine, + final String generatedClass, + final int generatedLine) { + this.yamlFile = yamlFile; + this.yamlLine = yamlLine; + this.generatedClass = generatedClass; + this.generatedLine = generatedLine; + } + + /** + * Non-positive lines normalise to {@link #UNRESOLVED}, so a {@code 0} from an older accessor + * cannot be mistaken for a real line. + * + * @param yamlFile rule file name, e.g. {@code vm.yaml} + * @param yamlLine 1-based line in that rule file + * @param generatedClass simple name of the generated class, e.g. {@code vm_L38_cpu_total$_tag} + * @param generatedLine 1-based line within that class's {@code .java} + * @return the coordinates of one generated file + */ + public static MalSourceRef of(final String yamlFile, + final int yamlLine, + final String generatedClass, + final int generatedLine) { + return new MalSourceRef( + yamlFile, + yamlLine > 0 ? yamlLine : UNRESOLVED, + generatedClass, + generatedLine > 0 ? generatedLine : UNRESOLVED); + } + + /** + * The rule anchor alone, before any class has been generated from it. + * + *

Two-phase by necessity, not by taste: a generated class name embeds the rule's line + * ({@code vm_L38_cpu_total}), so the anchor must exist BEFORE the class it will later name. + * {@link #inGeneratedClass} completes it once that name is known. + * + * @param yamlFile rule file name, e.g. {@code vm.yaml} + * @param yamlLine 1-based line in that rule file + * @return the operator coordinate, with no generated file yet + */ + public static MalSourceRef ofRule(final String yamlFile, final int yamlLine) { + return of(yamlFile, yamlLine, null, UNRESOLVED); + } + + /** + * Completes this anchor for one generated file. The rule half is carried over unchanged, which + * is what makes every artifact of a rule agree on its operator coordinate. + * + * @param generatedClass simple name of the generated class + * @param generatedLine 1-based line within that class's {@code .java} + * @return a ref describing that one generated file + */ + public MalSourceRef inGeneratedClass(final String generatedClass, final int generatedLine) { + return of(yamlFile, yamlLine, generatedClass, generatedLine); + } + + /** + * Parses the {@code "vm.yaml:38"} wire form. + * + *

This is the ONLY parser of that form, as {@link #describeYaml()} is its only renderer. + * The format survives because it crosses module boundaries as a single string; keeping both + * ends here is what stops the file and the line drifting apart, which is exactly what happened + * when two call sites each re-split it with their own {@code lastIndexOf(':')}. + * + * @param fused the rendered form, or {@code null} + * @return the rule anchor; an unparseable line yields {@link #UNRESOLVED} rather than throwing + */ + public static MalSourceRef parse(final String fused) { + if (fused == null) { + return ofRule(null, UNRESOLVED); + } + final int colonIdx = fused.lastIndexOf(':'); + if (colonIdx <= 0) { + return ofRule(fused, UNRESOLVED); + } + int line; + try { + line = Integer.parseInt(fused.substring(colonIdx + 1).trim()); + } catch (final NumberFormatException e) { + line = UNRESOLVED; + } + return ofRule(fused.substring(0, colonIdx), line); + } + + /** + * The {@code .java} this class's {@code SourceFile} attribute names. Deriving it here rather + * than at each call site is what keeps the attribute and the file written to disk in step — + * they disagreed for every closure companion until the sidecar was written. + * + * @return the source file name, or {@code null} when no class name is known + */ + public String generatedFileName() { + return generatedClass == null ? null : generatedClass + ".java"; + } + + /** + * @return the operator-facing location, e.g. {@code vm.yaml:38} + */ + public String describeYaml() { + return yamlFile + ":" + yamlLine; + } + + /** + * @return the machine-facing location, e.g. {@code vm_L38_cpu_total$_tag.java:9} + */ + public String describeGenerated() { + return generatedFileName() + ":" + generatedLine; + } + + /** + * Render a line for use inside a generated Java identifier. {@code -1} is not legal in an + * identifier, so an unresolved line becomes the literal {@code unknown} — still visible and + * greppable in a class name, but syntactically valid. + * + * @param line a line in either coordinate space + * @return the identifier-safe rendering + */ + public static String toIdentifierSegment(final int line) { + return line > 0 ? Integer.toString(line) : "unknown"; + } + + @Override + public String toString() { + return "MalSourceRef(" + describeYaml() + " -> " + describeGenerated() + ")"; + } +} diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/MetricsRule.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/MetricsRule.java index 0a10499f9cda..6f610967d3a2 100644 --- a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/MetricsRule.java +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/MetricsRule.java @@ -21,6 +21,8 @@ import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; +import lombok.EqualsAndHashCode; +import lombok.ToString; import lombok.NoArgsConstructor; import org.apache.skywalking.oap.meter.analyzer.v2.MetricRuleConfig; @@ -34,4 +36,16 @@ public class MetricsRule implements MetricRuleConfig.RuleConfig { private String name; private String exp; + /** + * Source anchors, stamped by the loader from {@code MalYamlLineIndex} — not YAML-bound keys. + * Excluded from equals/hashCode/toString so a rule's identity stays its content: the + * runtime-rule delta classifier compares parsed rules, and a rule that merely moved down the + * file must not read as changed. + */ + @EqualsAndHashCode.Exclude + @ToString.Exclude + private int lineNo; + @EqualsAndHashCode.Exclude + @ToString.Exclude + private int expLine; } diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/Rule.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/Rule.java index 47497a964f39..827fb33697ca 100644 --- a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/Rule.java +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/Rule.java @@ -19,6 +19,8 @@ package org.apache.skywalking.oap.meter.analyzer.v2.prometheus.rule; import lombok.Data; +import lombok.EqualsAndHashCode; +import lombok.ToString; import lombok.NoArgsConstructor; import org.apache.skywalking.oap.meter.analyzer.v2.MetricRuleConfig; import org.apache.skywalking.oap.server.core.analysis.LayerDefinition; @@ -43,6 +45,20 @@ public class Rule implements MetricRuleConfig { * rule file is self-describing for any custom layers it references. */ private List layerDefinitions; + /** + * File-level source anchors, stamped by the loader from {@code MalYamlLineIndex} — not + * YAML-bound keys. Excluded from equals/hashCode/toString for the same reason as + * {@link MetricsRule}'s: a rule file that merely shifted must not read as content-changed. + */ + @EqualsAndHashCode.Exclude + @ToString.Exclude + private int filterLine; + @EqualsAndHashCode.Exclude + @ToString.Exclude + private int expPrefixLine; + @EqualsAndHashCode.Exclude + @ToString.Exclude + private int expSuffixLine; @Override public String getSourceName() { diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/RuleSourceLines.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/RuleSourceLines.java new file mode 100644 index 000000000000..2a56d84143a6 --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/RuleSourceLines.java @@ -0,0 +1,71 @@ +/* + * 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.meter.analyzer.v2.prometheus.rule; + +import java.util.List; +import org.apache.skywalking.oap.meter.analyzer.v2.MalYamlLineIndex; + +/** + * Stamps YAML source anchors onto a parsed {@link Rule}. + * + *

Exists so every loader that binds a rule file — the disk loader, the runtime-rule hot-update + * applier, and the class-generation tooling — assigns lines the same way. They previously agreed + * only by coincidence: the test harness scanned for {@code name:} text while production used the + * rules-list index, so the two produced different numbers for the same rule. + * + *

Callers MUST pass the exact text the rule was bound from. Re-reading the file would risk + * indexing a different revision than the one in memory. + */ +public final class RuleSourceLines { + + private RuleSourceLines() { + } + + /** + * Assign file-level and per-rule anchors. No-op when either argument is null, and individual + * anchors stay {@code 0} when the document doesn't declare them. + * + * @param rule parsed rule to stamp in place + * @param yamlContent the exact text {@code rule} was bound from + */ + public static void assign(final Rule rule, final String yamlContent) { + if (rule == null || yamlContent == null) { + return; + } + final MalYamlLineIndex index = MalYamlLineIndex.index(yamlContent); + rule.setFilterLine(index.getFilterLine()); + rule.setExpPrefixLine(index.getExpPrefixLine()); + rule.setExpSuffixLine(index.getExpSuffixLine()); + + final List rules = rule.getMetricsRules(); + if (rules == null) { + return; + } + // Positional: the index walks the same sequence snakeyaml bound, in the same order. + for (int i = 0; i < rules.size(); i++) { + final MetricsRule metricsRule = rules.get(i); + if (metricsRule == null) { + continue; + } + final MalYamlLineIndex.RuleLines lines = index.rule(i); + metricsRule.setLineNo(lines.getEntryLine()); + metricsRule.setExpLine(lines.getExpLine()); + } + } +} diff --git a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/Rules.java b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/Rules.java index 4b0e369e1e07..439c0e55d3f4 100644 --- a/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/Rules.java +++ b/oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/prometheus/rule/Rules.java @@ -18,11 +18,10 @@ package org.apache.skywalking.oap.meter.analyzer.v2.prometheus.rule; -import java.io.ByteArrayInputStream; import java.io.File; import java.io.IOException; -import java.io.InputStreamReader; +import java.io.StringReader; import java.io.Reader; import java.nio.charset.StandardCharsets; import java.nio.file.FileSystems; @@ -145,12 +144,16 @@ private static List loadInternal(final String path, List enabledRu } private static Rule parseRule(final String ruleName, final byte[] bytes) { - try (Reader r = new InputStreamReader(new ByteArrayInputStream(bytes), StandardCharsets.UTF_8)) { + // Decode once and bind from the String: RuleSourceLines re-composes the SAME text to read + // positional marks, which snakeyaml's bean binding discards. + final String text = new String(bytes, StandardCharsets.UTF_8); + try (Reader r = new StringReader(text)) { Rule rule = new Yaml().loadAs(r, Rule.class); if (rule == null) { return null; } rule.setName(ruleName); + RuleSourceLines.assign(rule, text); registerInlineLayers(ruleName, rule); return rule; } catch (IOException e) { diff --git a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/MalYamlLineIndexTest.java b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/MalYamlLineIndexTest.java new file mode 100644 index 000000000000..79917612abc0 --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/MalYamlLineIndexTest.java @@ -0,0 +1,127 @@ +/* + * 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.meter.analyzer.v2; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Pins the 1-based line contract. SnakeYAML marks are 0-based, so an off-by-one here would + * silently mis-attribute every generated class label and every debug sample. + */ +class MalYamlLineIndexTest { + + /** Mirrors the shape of a shipped rule file: file-level keys, then a rules sequence. */ + private static final String YAML = + "# comment line 1\n" // 1 + + "\n" // 2 + + "filter: \"{ tags -> true }\"\n" // 3 + + "expSuffix: service(['host'])\n" // 4 + + "metricPrefix: meter_vm\n" // 5 + + "metricsRules:\n" // 6 + + "\n" // 7 + + " # a comment inside the list\n" // 8 + + " - name: cpu_total\n" // 9 + + " exp: node_cpu.sum(['host'])\n" // 10 + + " - name: mem_used\n" // 11 + + " exp: node_mem.sum(['host'])\n"; // 12 + + @Test + void resolvesFileLevelAnchors() { + final MalYamlLineIndex index = MalYamlLineIndex.index(YAML); + + assertEquals(3, index.getFilterLine()); + assertEquals(4, index.getExpSuffixLine()); + // Not declared in this document. + assertEquals(0, index.getExpPrefixLine()); + } + + @Test + void resolvesPerRuleAnchorsSkippingBlanksAndComments() { + final MalYamlLineIndex index = MalYamlLineIndex.index(YAML); + + // The entry anchor is the `- name:` line, not the `metricsRules:` key and not the + // blank/comment lines between them. + assertEquals(9, index.rule(0).getEntryLine()); + assertEquals(10, index.rule(0).getExpLine()); + assertEquals(11, index.rule(1).getEntryLine()); + assertEquals(12, index.rule(1).getExpLine()); + } + + @Test + void outOfRangeRuleIsZeroNotAnError() { + final MalYamlLineIndex index = MalYamlLineIndex.index(YAML); + + assertEquals(0, index.rule(99).getEntryLine()); + assertEquals(0, index.rule(-1).getExpLine()); + } + + @Test + void honoursAnAlternateRulesKey() { + // Zabbix rule files hold their rules under `metrics:` rather than `metricsRules:`. + final String zabbix = "metricPrefix: meter_zb\n" // 1 + + "metrics:\n" // 2 + + " - name: cpu\n" // 3 + + " exp: agent_cpu.sum(['host'])\n"; // 4 + + assertEquals(3, MalYamlLineIndex.index(zabbix, "metrics").rule(0).getEntryLine()); + assertEquals(4, MalYamlLineIndex.index(zabbix, "metrics").rule(0).getExpLine()); + // The default key finds nothing in that document — zeros, not an exception. + assertEquals(0, MalYamlLineIndex.index(zabbix).rule(0).getEntryLine()); + } + + @Test + void malformedOrEmptyInputDegradesToZeros() { + // A missing line number must never stop a rule from compiling. + assertEquals(0, MalYamlLineIndex.index(null).getFilterLine()); + assertEquals(0, MalYamlLineIndex.index("").getFilterLine()); + assertEquals(0, MalYamlLineIndex.index("- not: a mapping root").getFilterLine()); + assertEquals(0, MalYamlLineIndex.index("key: [unclosed").getFilterLine()); + } + + @Test + void blockScalarsAnchorOnTheirKeyLineNotTheirContent() { + // Shipped rules use both folded (`>`) and literal (`|-`) scalars, and a real one spans + // 30 lines (meter-analyzer-config/network-profiling.yaml). Everything the fragment + // contributes is attributed to the KEY line, which is the stable anchor an operator + // navigates to — the content lines have no individual meaning to a chain stage. + final String yaml = + "expSuffix: |-\n" // 1 key + + " service(['host'],\n" // 2 content + + " Layer.GENERAL)\n" // 3 content + + "metricPrefix: meter_vm\n" // 4 + + "metricsRules:\n" // 5 + + " - name: cpu\n" // 6 + + " exp: >\n" // 7 key + + " node_cpu\n" // 8 content + + " .sum(['host'])\n" // 9 content + + " - name: mem\n" // 10 + + " exp: node_mem\n"; // 11 + + final MalYamlLineIndex index = MalYamlLineIndex.index(yaml); + + assertEquals(1, index.getExpSuffixLine(), "multi-line expSuffix anchors on its key"); + assertEquals(6, index.rule(0).getEntryLine()); + assertEquals(7, index.rule(0).getExpLine(), "folded exp anchors on its key, not line 8"); + // The rule AFTER a multi-line scalar must not be shifted by the scalar's height. + assertEquals(10, index.rule(1).getEntryLine()); + assertEquals(11, index.rule(1).getExpLine()); + } +} diff --git a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalClassAttributes.java b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalClassAttributes.java new file mode 100644 index 000000000000..f88ca524ee60 --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalClassAttributes.java @@ -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.skywalking.oap.meter.analyzer.v2.compiler; + +import java.io.BufferedInputStream; +import java.io.DataInputStream; +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.util.ArrayList; +import java.util.List; +import javassist.bytecode.ClassFile; +import javassist.bytecode.CodeAttribute; +import javassist.bytecode.LineNumberAttribute; +import javassist.bytecode.MethodInfo; + +/** + * Reads debug attributes back out of a generated {@code .class}. + * + *

Asserting on the emitted bytecode rather than on the codegen's inputs is the point: the + * inputs were correct before this change too, and the attribute was still wrong. + */ +final class MalClassAttributes { + + private MalClassAttributes() { + } + + /** + * @param classFile a generated class file on disk + * @return every {@code LineNumberTable} entry as {@code {start_pc, line_number}}, across all + * methods; empty when the class carries no table at all + * @throws IOException if the class file cannot be read + */ + static List lineNumberTableOf(final File classFile) throws IOException { + final List out = new ArrayList<>(); + try (DataInputStream in = new DataInputStream( + new BufferedInputStream(Files.newInputStream(classFile.toPath())))) { + final ClassFile cf = new ClassFile(in); + for (final MethodInfo mi : cf.getMethods()) { + final CodeAttribute code = mi.getCodeAttribute(); + if (code == null) { + continue; + } + final LineNumberAttribute lna = + (LineNumberAttribute) code.getAttribute(LineNumberAttribute.tag); + if (lna == null) { + continue; + } + for (int i = 0; i < lna.tableLength(); i++) { + out.add(new int[]{lna.startPc(i), lna.lineNumber(i)}); + } + } + } + return out; + } + + /** + * @param classFile a generated class file on disk + * @return the {@code SourceFile} attribute value, or {@code null} when absent + * @throws IOException if the class file cannot be read + */ + static String sourceFileOf(final File classFile) throws IOException { + try (DataInputStream in = new DataInputStream( + new BufferedInputStream(Files.newInputStream(classFile.toPath())))) { + return new ClassFile(in).getSourceFile(); + } + } +} diff --git a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalClosureLineAttributionTest.java b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalClosureLineAttributionTest.java new file mode 100644 index 000000000000..f4884bf020aa --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalClosureLineAttributionTest.java @@ -0,0 +1,175 @@ +/* + * 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.meter.analyzer.v2.compiler; + +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.List; +import javassist.ClassPool; +import org.apache.skywalking.oap.meter.analyzer.v2.MetricRuleConfig; +import org.apache.skywalking.oap.meter.analyzer.v2.prometheus.rule.Rule; +import org.apache.skywalking.oap.meter.analyzer.v2.prometheus.rule.RuleSourceLines; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.yaml.snakeyaml.Yaml; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Both coordinate spaces, for closures in all three authoring positions. + * + *

A closure is the one construct where a rule stops being a single file — it becomes its own + * class — and it is also the construct most often written OUTSIDE the rule's own {@code exp:}. In + * the shipped rule set a {@code tag({...})} in a file-level {@code expPrefix:} or {@code + * expSuffix:} is the dominant pattern, not an edge case: {@code otel-rules/istio-controlplane.yaml} + * carries one on line 31, and every activemq / clickhouse / aws-* rule does the same. + * + *

That combination is exactly where an implementation resolving per-RULE instead of per-STAGE + * stays plausible while being wrong: it reports the rule's {@code exp:} line for a closure the + * operator wrote twenty lines earlier, and the line it names does exist, so nothing looks broken. + */ +class MalClosureLineAttributionTest { + + /** + * Closures in all three positions, on deliberately distinct lines. If any assertion below + * could pass by reporting a neighbouring line, the fixture is not doing its job. + */ + private static final String YAML = + "expPrefix: tag({tags -> tags.a = 'p'})\n" // 1 + + "expSuffix: tag({tags -> tags.b = 's'}).service(['host'], Layer.GENERAL)\n" // 2 + + "metricPrefix: meter_vm\n" // 3 + + "metricsRules:\n" // 4 + + " - name: cpu\n" // 5 + + " exp: node_cpu.tag({tags -> tags.c = 'e'}).sum(['host'])\n"; // 6 + + private static final int PREFIX_LINE = 1; + private static final int SUFFIX_LINE = 2; + private static final int EXP_LINE = 6; + + @TempDir + File outputDir; + + private Rule rule; + private String formatted; + + @BeforeEach + void setUp() { + rule = new Yaml().loadAs(YAML, Rule.class); + rule.setName("vm"); + RuleSourceLines.assign(rule, YAML); + + final MetricRuleConfig.RuleConfig r = rule.getMetricsRules().get(0); + // Exactly what MetricConvert.formatExp splices together. + formatted = "(" + MALScriptParser.injectExpPrefix(r.getExp(), rule.getExpPrefix()) + + ")." + rule.getExpSuffix(); + } + + @Test + void theFixtureItselfAnchorsWhereTheCommentsClaim() { + assertEquals(PREFIX_LINE, rule.getExpPrefixLine()); + assertEquals(SUFFIX_LINE, rule.getExpSuffixLine()); + assertEquals(EXP_LINE, rule.getMetricsRules().get(0).getExpLine()); + } + + @Test + void aClosureFromEachPositionGetsItsOwnGeneratedFileAndItsOwnResolvableLine() throws Exception { + compileFormatted(); + + final File[] companions = outputDir.listFiles( + (dir, name) -> name.contains("$") && name.endsWith(".class")); + assertNotNull(companions, "nothing written to " + outputDir); + assertEquals(3, companions.length, + "one rule, three closures (prefix + exp + suffix) — Javassist cannot emit lambdas, " + + "so each is its own class file"); + + // Completing the matrix: the YAML-line test above proves each POSITION resolves to its own + // operator line; this proves each position also lands in its own GENERATED file with a + // resolvable line there. Companions are matched by the marker each closure body writes, so + // the assertion does not depend on the order codegen happens to assign field names in. + assertGeneratedCoordinates("put(\"a\"", "expPrefix"); + assertGeneratedCoordinates("put(\"c\"", "exp"); + assertGeneratedCoordinates("put(\"b\"", "expSuffix"); + } + + /** + * The generated half of one matrix cell: the companion carrying {@code marker} has its own + * {@code .java}, its {@code SourceFile} names that file, and its single {@code LineNumberTable} + * entry resolves to the SAM signature inside it. + */ + private void assertGeneratedCoordinates(final String marker, + final String position) throws Exception { + File found = null; + for (final File companion : outputDir.listFiles( + (dir, name) -> name.contains("$") && name.endsWith(".class"))) { + final File sidecar = new File( + outputDir, companion.getName().replace(".class", ".java")); + if (sidecar.isFile() && new String(Files.readAllBytes(sidecar.toPath()), + StandardCharsets.UTF_8).contains(marker)) { + found = companion; + break; + } + } + assertNotNull(found, "no companion sidecar carries the " + position + " closure body"); + + final File sidecar = new File(outputDir, found.getName().replace(".class", ".java")); + assertEquals(found.getName().replace(".class", ".java"), + MalClassAttributes.sourceFileOf(found), + "SourceFile must name the companion's own file, for " + position); + + final List table = MalClassAttributes.lineNumberTableOf(found); + assertEquals(1, table.size(), "one entry for the " + position + " companion"); + + final int line = table.get(0)[1]; + final List lines = + Files.readAllLines(sidecar.toPath(), StandardCharsets.UTF_8); + assertTrue(line >= 1 && line <= lines.size(), + "line " + line + " is outside the " + position + " companion's own source"); + final String target = lines.get(line - 1); + assertTrue(target.contains("public ") && target.endsWith("{"), + "the " + position + " companion's line should hold its SAM signature but was: '" + + target + "'"); + } + + private void compileFormatted() throws Exception { + final MALClassGenerator generator = new MALClassGenerator(new ClassPool(true)); + generator.setClassOutputDir(outputDir); + generator.setYamlSource("vm.yaml:" + EXP_LINE); + generator.setClassNameHint("cpu"); + generator.compile("meter_vm_cpu", formatted); + } + + @Test + void theYamlAnchorIsSharedAcrossTheGeneratedFilesThatOneRuleProduces() { + // The cardinality the shared structure exists to express: the operator coordinate is fixed + // per rule, while the generated coordinate differs per file. + final MalSourceRef main = MalSourceRef.of("vm.yaml", EXP_LINE, "vm_L6_cpu", 11); + final MalSourceRef tag = MalSourceRef.of("vm.yaml", EXP_LINE, "vm_L6_cpu$_tag", 9); + final MalSourceRef tag2 = MalSourceRef.of("vm.yaml", EXP_LINE, "vm_L6_cpu$_tag_2", 9); + + assertEquals(main.describeYaml(), tag.describeYaml()); + assertEquals(tag.describeYaml(), tag2.describeYaml()); + assertNotEquals(tag.generatedFileName(), tag2.generatedFileName()); + assertEquals("vm_L6_cpu$_tag.java", tag.generatedFileName()); + } +} diff --git a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalCompanionSourceTest.java b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalCompanionSourceTest.java new file mode 100644 index 000000000000..2def3ca8c5ca --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalCompanionSourceTest.java @@ -0,0 +1,180 @@ +/* + * 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.meter.analyzer.v2.compiler; + +import java.io.BufferedInputStream; +import java.io.DataInputStream; +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.ArrayList; +import java.util.List; +import javassist.ClassPool; +import javassist.bytecode.ClassFile; +import javassist.bytecode.CodeAttribute; +import javassist.bytecode.LineNumberAttribute; +import javassist.bytecode.MethodInfo; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * A closure companion is its own class file, so it must carry its own generated-source + * coordinates: its own {@code .java}, and a line into THAT file. + * + *

Javassist's {@code ClassFile} constructor always installs a {@code SourceFile} attribute + * naming {@code .java}, so a companion has ALWAYS claimed a source file. Until the + * sidecar was written that name dangled — it pointed at a file nobody ever created, which reads + * as correct to an IDE right up until it fails to open it. These tests pin that the name resolves. + */ +class MalCompanionSourceTest { + + /** Rule YAML anchor, in the fused form the generator is configured with. */ + private static final String YAML_SOURCE = "vm.yaml:38"; + + @TempDir + File outputDir; + + private MALClassGenerator generator; + + @BeforeEach + void setUp() { + generator = new MALClassGenerator(new ClassPool(true)); + generator.setClassOutputDir(outputDir); + generator.setYamlSource(YAML_SOURCE); + } + + @Test + void aCompanionGetsItsOwnSidecarSoItsSourceFileNameResolves() throws Exception { + generator.setClassNameHint("cpu_total"); + generator.compile("meter_vm_cpu", "metric.tag({ tags -> tags.service = 'svc1' })"); + + final File companion = findCompanionClass(); + final File sidecar = new File( + outputDir, companion.getName().replace(".class", ".java")); + assertTrue(sidecar.isFile(), + "companion .class exists but its SourceFile names a file that was never written: " + + sidecar.getName()); + } + + @Test + void theCompanionLineNumberEntryPointsAtItsOwnSamSignature() throws Exception { + generator.setClassNameHint("cpu_total"); + generator.compile("meter_vm_cpu", "metric.tag({ tags -> tags.service = 'svc1' })"); + + final File companion = findCompanionClass(); + final List sidecarLines = Files.readAllLines( + new File(outputDir, companion.getName().replace(".class", ".java")).toPath(), + StandardCharsets.UTF_8); + + final List table = MalClassAttributes.lineNumberTableOf(companion); + assertEquals(1, table.size(), + "a companion carries exactly ONE entry: the per-statement scan cannot find real " + + "boundaries in a closure body, so more than one entry would be invented"); + assertEquals(0, table.get(0)[0], "the single entry covers the method from pc 0"); + + // The decisive assertion. A line number is only meaningful against the file it indexes, + // so resolve it and check what is actually there. This fails if the sidecar wrapper ever + // gains or loses a line without COMPANION_SAM_LINE_IN_CLASS being updated to match. + final int line = table.get(0)[1]; + assertTrue(line >= 1 && line <= sidecarLines.size(), + "line " + line + " is outside the sidecar (" + sidecarLines.size() + " lines)"); + final String target = sidecarLines.get(line - 1); + assertTrue(target.contains("public ") && target.contains("(") && target.endsWith("{"), + "line " + line + " should hold the SAM signature but was: '" + target + "'"); + } + + @Test + void eachCompanionOfOneRuleIsItsOwnFileWhileTheYamlAnchorStaysShared() throws Exception { + generator.setClassNameHint("multi"); + generator.compile("meter_vm_multi", + "metric.tag({ tags -> tags.a = 'b' }).tag({ tags -> tags.c = 'd' })"); + + final File[] companions = outputDir.listFiles( + (dir, name) -> name.contains("$") && name.endsWith(".class")); + assertNotNull(companions); + assertEquals(2, companions.length, + "one rule, two closures — Javassist cannot emit lambdas, so each is its own class"); + + // Distinct generated files ... + assertTrue(!companions[0].getName().equals(companions[1].getName())); + for (final File companion : companions) { + assertTrue(new File(outputDir, companion.getName().replace(".class", ".java")).isFile(), + "every generated class file needs its own source file: " + companion.getName()); + } + + // ... all sharing the one operator anchor. This is the cardinality the shared structure + // exists to express: 1 YAML line, N generated files. + final MalSourceRef first = MalSourceRef.of("vm.yaml", 38, "vm_L38_multi$_tag", 9); + final MalSourceRef second = MalSourceRef.of("vm.yaml", 38, "vm_L38_multi$_tag_2", 9); + assertEquals(first.describeYaml(), second.describeYaml(), "same rule, same anchor"); + assertEquals("vm_L38_multi$_tag.java", first.generatedFileName()); + assertEquals("vm_L38_multi$_tag_2.java", second.generatedFileName()); + } + + @Test + void anUnresolvedGeneratedLineIsNeverWrittenIntoTheTable() { + // line_number is an unsigned u2, so UNRESOLVED (-1) would serialize as 65535 and read as + // a real line. Omitting the attribute is the only honest encoding of "unknown". + final MalSourceRef ref = MalSourceRef.of("vm.yaml", 38, "vm_L38_x$_tag", 0); + + assertEquals(MalSourceRef.UNRESOLVED, ref.getGeneratedLine()); + assertEquals("vm.yaml:38", ref.describeYaml(), "the yaml anchor survives independently"); + } + + private File findCompanionClass() { + final File[] companions = outputDir.listFiles( + (dir, name) -> name.contains("$") && name.endsWith(".class")); + assertNotNull(companions, "nothing written to " + outputDir); + assertEquals(1, companions.length, "expected exactly one companion"); + return companions[0]; + } + + /** Minimal {@code LineNumberTable} reader: returns {@code {start_pc, line_number}} pairs. */ + private static final class LineNumberTables { + + static List of(final File classFile) throws IOException { + final List out = new ArrayList<>(); + try (DataInputStream in = new DataInputStream( + new BufferedInputStream(Files.newInputStream(classFile.toPath())))) { + final ClassFile cf = new ClassFile(in); + for (final MethodInfo mi : cf.getMethods()) { + final CodeAttribute code = mi.getCodeAttribute(); + if (code == null) { + continue; + } + final LineNumberAttribute lna = (LineNumberAttribute) + code.getAttribute(LineNumberAttribute.tag); + if (lna == null) { + continue; + } + for (int i = 0; i < lna.tableLength(); i++) { + out.add(new int[]{lna.startPc(i), lna.lineNumber(i)}); + } + } + } + return out; + } + } +} diff --git a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalGeneratedSourceLinesTest.java b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalGeneratedSourceLinesTest.java new file mode 100644 index 000000000000..5900974e1b50 --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalGeneratedSourceLinesTest.java @@ -0,0 +1,92 @@ +/* + * 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.meter.analyzer.v2.compiler; + +import java.util.Arrays; +import java.util.List; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * The boundary list must match, one-for-one and in order, the entries + * {@code MALBytecodeHelper.addLineNumberTable} derives from the bytecode. A mismatch would put + * real-looking but wrong line numbers in the table, which is worse than the sequential ordinals + * it replaces. + */ +class MalGeneratedSourceLinesTest { + + /** Verbatim shape of a real generated run() body — the vm.yaml cpu_total_percentage rule. */ + private static final String RUN_BODY = + "public SampleFamily run(java.util.Map samples) {\n" // 1 signature + + " SampleFamily _node_cpu = ((SampleFamily) samples.get(\"x\"));\n" // 2 declare+store + + " _node_cpu = _node_cpu.multiply(Long.valueOf(100L));\n" // 3 store + + " _node_cpu = _node_cpu.tagNotEqual(new String[]{\"m\", \"i\"});\n" // 4 store + + " _node_cpu = _node_cpu.sum(java.util.Arrays.asList(new String[]{\"h\"}));\n" // 5 + + " _node_cpu = _node_cpu.rate(\"PT1M\");\n" // 6 store + + " _node_cpu = _node_cpu.service(java.util.Arrays.asList(x), y);\n" // 7 store + + " return _node_cpu;\n" // 8 return + + "}\n"; // 9 + + @Test + void findsEveryAssignmentPlusTheReturn() { + final List lines = MalGeneratedSourceLines.statementLinesOf(RUN_BODY); + + // 6 assignments (lines 2-7) + the return (line 8) = 7 boundaries, matching the 7 entries + // javap shows for this rule today. + assertEquals(Arrays.asList(2, 3, 4, 5, 6, 7, 8), lines); + } + + @Test + void skipsProbeCallsWhichEmitNoStore() { + // With SW_DSL_DEBUGGING_INJECTION_ENABLED the codegen interleaves probe calls. They add + // source lines but NO bytecode boundary, which is precisely why a fixed "statement N is at + // line N+1" offset cannot be used. + final String withProbes = + "public SampleFamily run(java.util.Map samples) {\n" // 1 + + " MALDebug.captureInput(this.debug, \"r\", \"m\", _v);\n" // 2 no store + + " SampleFamily _v = ((SampleFamily) samples.get(\"x\"));\n" // 3 store + + " MALDebug.captureStage(this.debug, \"r\", \"sum\", _v);\n" // 4 no store + + " _v = _v.sum(java.util.Arrays.asList(new String[]{\"h\"}));\n" // 5 store + + " return _v;\n" // 6 return + + "}\n"; + + assertEquals(Arrays.asList(3, 5, 6), MalGeneratedSourceLines.statementLinesOf(withProbes)); + } + + @Test + void ignoresNonResultLocalsAndBareExpressions() { + final String mixed = + "public SampleFamily run(java.util.Map samples) {\n" // 1 + + " java.util.List other = new java.util.ArrayList();\n" // 2 not a _ target + + " other.add(\"x\");\n" // 3 bare expression + + " SampleFamily _v = SampleFamily.EMPTY;\n" // 4 store + + " return _v;\n" // 5 return + + "}\n"; + + assertEquals(Arrays.asList(4, 5), MalGeneratedSourceLines.statementLinesOf(mixed)); + } + + @Test + void emptyInputIsEmptyNotAnError() { + assertTrue(MalGeneratedSourceLines.statementLinesOf(null).isEmpty()); + assertTrue(MalGeneratedSourceLines.statementLinesOf("").isEmpty()); + } +} diff --git a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalLineAttributionTest.java b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalLineAttributionTest.java new file mode 100644 index 000000000000..363f5efcece7 --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalLineAttributionTest.java @@ -0,0 +1,181 @@ +/* + * 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.meter.analyzer.v2.compiler; + +import java.io.BufferedInputStream; +import java.io.DataInputStream; +import java.io.File; +import java.io.FileInputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.ArrayList; +import java.util.List; +import javassist.ClassPool; +import javassist.bytecode.ClassFile; +import javassist.bytecode.CodeAttribute; +import javassist.bytecode.LineNumberAttribute; +import javassist.bytecode.MethodInfo; +import org.apache.skywalking.oap.meter.analyzer.v2.dsl.MalExpression; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * End-to-end guard on the generated artifact's line attribution. + * + *

The unit tests for {@link MalYamlLineIndex} and {@link MalGeneratedSourceLines} check those + * pieces in isolation. This one checks the thing that actually ships: that the {@code SourceFile} + * attribute names the file that was really written, and that every {@code LineNumberTable} entry + * indexes a line of THAT file which genuinely holds a statement. + * + *

It exists because the arithmetic converting a method-relative line to a class-relative one + * depends on the class wrapper's shape — the number of closure fields and whether debug injection + * is enabled. That is a constant derived by hand, so it needs a test that would fail if the + * wrapper ever grows a line. Before this work the same code emitted {@code SourceFile} naming a + * file that never existed and a table of statement ordinals, and nothing noticed. + */ +class MalLineAttributionTest { + + @TempDir + File outputDir; + + private MALClassGenerator generator; + + @BeforeEach + void setUp() { + org.apache.skywalking.oap.server.core.dsldebug.DSLDebugCodegenSwitch.resetInjection(); + generator = new MALClassGenerator(new ClassPool(true)); + generator.setClassOutputDir(outputDir); + } + + @Test + void sourceFileNamesTheFileThatWasActuallyWritten() throws Exception { + compileMultiStageRule(); + + final File javaFile = writtenFile(".java"); + final ClassFile classFile = readClassFile(writtenFile(".class")); + + // The whole contract of SourceFile: an IDE resolves a frame by opening exactly this name + // next to the class. It used to read "(vm.yaml:0)cpu_total.java" while the file on disk + // was "vm_L0_cpu_total.java", so it could never resolve. + assertEquals(javaFile.getName(), classFile.getSourceFile()); + } + + @Test + void classNameCarriesTheRealYamlLine() throws Exception { + compileMultiStageRule(); + + // 37 is what the caller supplied as the rule's line; it must survive into the label + // rather than being replaced by the rule's position in the list. + assertTrue(writtenFile(".class").getName().contains("_L37_"), + "expected the YAML line in the class name, got: " + writtenFile(".class").getName()); + } + + @Test + void everyLineNumberEntryPointsAtAStatementInThatFile() throws Exception { + compileMultiStageRule(); + + final List sourceLines = + Files.readAllLines(writtenFile(".java").toPath(), StandardCharsets.UTF_8); + final List tableLines = lineNumbersOf(readClassFile(writtenFile(".class")), "run"); + + assertTrue(tableLines.size() >= 2, + "expected several boundaries for a multi-stage rule, got " + tableLines.size()); + + for (final int line : tableLines) { + assertTrue(line >= 1 && line <= sourceLines.size(), + "line " + line + " is outside the generated file (1.." + sourceLines.size() + ")"); + final String text = sourceLines.get(line - 1).trim(); + // A boundary is either a result assignment or the terminating return — never a blank, + // a brace, or the class declaration, which is what an off-by-N would land on. + assertTrue(text.endsWith(";"), + "line " + line + " should hold a statement but was: '" + text + "'"); + } + } + + @Test + void theLastBoundaryIsTheReturn() throws Exception { + compileMultiStageRule(); + + final List sourceLines = + Files.readAllLines(writtenFile(".java").toPath(), StandardCharsets.UTF_8); + final List tableLines = lineNumbersOf(readClassFile(writtenFile(".class")), "run"); + + // Pins the ordering end-to-end: the final bytecode boundary must correspond to the final + // statement of the method, so any drift in the middle would shift this off the return. + final String last = sourceLines.get(tableLines.get(tableLines.size() - 1) - 1).trim(); + assertTrue(last.startsWith("return "), + "last boundary should be the return, was: '" + last + "'"); + } + + /** A chain with several stages, so the table has more than one entry to get wrong. */ + private void compileMultiStageRule() throws Exception { + generator.setClassNameHint("cpu_total"); + generator.setYamlSource("vm.yaml:37"); + try { + final MalExpression expr = generator.compile( + "meter_vm_cpu_total", + "(node_cpu_seconds_total * 100).tagNotEqual('mode', 'idle')" + + ".sum(['host']).rate('PT1M')"); + assertNotNull(expr); + } finally { + generator.setClassNameHint(null); + generator.setYamlSource(null); + } + } + + private File writtenFile(final String suffix) { + final File[] matches = outputDir.listFiles( + (dir, name) -> name.endsWith(suffix) && name.contains("cpu_total")); + assertNotNull(matches, "no files written to " + outputDir); + assertTrue(matches.length > 0, "no " + suffix + " written to " + outputDir); + return matches[0]; + } + + private static ClassFile readClassFile(final File file) throws Exception { + try (DataInputStream in = + new DataInputStream(new BufferedInputStream(new FileInputStream(file)))) { + return new ClassFile(in); + } + } + + private static List lineNumbersOf(final ClassFile classFile, final String methodName) { + final List out = new ArrayList<>(); + for (final MethodInfo method : classFile.getMethods()) { + if (!methodName.equals(method.getName())) { + continue; + } + final CodeAttribute code = method.getCodeAttribute(); + if (code == null) { + continue; + } + final LineNumberAttribute table = + (LineNumberAttribute) code.getAttribute(LineNumberAttribute.tag); + assertNotNull(table, "no LineNumberTable on " + methodName + "()"); + for (int i = 0; i < table.tableLength(); i++) { + out.add(table.lineNumber(i)); + } + } + return out; + } +} diff --git a/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalSourceRefTest.java b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalSourceRefTest.java new file mode 100644 index 000000000000..8653051d50f9 --- /dev/null +++ b/oap-server/analyzer/meter-analyzer/src/test/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MalSourceRefTest.java @@ -0,0 +1,79 @@ +/* + * 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.meter.analyzer.v2.compiler; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +/** + * {@code "vm.yaml:38"} still crosses module boundaries as one string, so it has a renderer and a + * parser. Both live on this type, and these tests pin that they agree. + * + *

They did not, before: the string was concatenated by hand in {@code MetricConvert} and split + * by hand in two places in {@code MALBytecodeHelper}, each with its own {@code lastIndexOf(':')}. + * Three independent implementations of one format is how a file and a line drift apart. + */ +class MalSourceRefTest { + + @Test + void renderAndParseAreInverses() { + final MalSourceRef anchor = MalSourceRef.ofRule("vm.yaml", 38); + final MalSourceRef roundTripped = MalSourceRef.parse(anchor.describeYaml()); + + assertEquals("vm.yaml:38", anchor.describeYaml()); + assertEquals(anchor.getYamlFile(), roundTripped.getYamlFile()); + assertEquals(anchor.getYamlLine(), roundTripped.getYamlLine()); + } + + @Test + void anUnresolvedLineSurvivesTheRoundTripAsMinusOneNotZero() { + // -1 must stay visible: it propagates into the class name as "unknown" and marks a + // resolution failure worth chasing. A 0 would read as "not applicable" and vanish. + final MalSourceRef anchor = MalSourceRef.ofRule("vm.yaml", 0); + + assertEquals("vm.yaml:-1", anchor.describeYaml()); + assertEquals(MalSourceRef.UNRESOLVED, MalSourceRef.parse(anchor.describeYaml()).getYamlLine()); + assertEquals("unknown", MalSourceRef.toIdentifierSegment(anchor.getYamlLine())); + } + + @Test + void aFileNameWithNoLineOrAGarbageLineDegradesRatherThanThrowing() { + assertEquals("vm.yaml", MalSourceRef.parse("vm.yaml").getYamlFile()); + assertEquals(MalSourceRef.UNRESOLVED, MalSourceRef.parse("vm.yaml").getYamlLine()); + assertEquals(MalSourceRef.UNRESOLVED, MalSourceRef.parse("vm.yaml:notanumber").getYamlLine()); + assertEquals("vm.yaml", MalSourceRef.parse("vm.yaml:notanumber").getYamlFile()); + assertNull(MalSourceRef.parse(null).getYamlFile()); + } + + @Test + void completingAnAnchorKeepsTheRuleHalfSoEveryGeneratedFileAgreesOnIt() { + final MalSourceRef anchor = MalSourceRef.ofRule("vm.yaml", 38); + final MalSourceRef main = anchor.inGeneratedClass("vm_L38_cpu", 11); + final MalSourceRef tag = anchor.inGeneratedClass("vm_L38_cpu$_tag", 9); + + // One rule, N generated files: the operator half is shared, the machine half is not. + assertEquals(main.describeYaml(), tag.describeYaml()); + assertEquals("vm_L38_cpu.java:11", main.describeGenerated()); + assertEquals("vm_L38_cpu$_tag.java:9", tag.describeGenerated()); + // The anchor itself is unchanged — completing it does not mutate it. + assertNull(anchor.getGeneratedClass()); + } +} diff --git a/oap-server/oal-rt/src/main/java/org/apache/skywalking/oal/v2/generator/OALClassGeneratorV2.java b/oap-server/oal-rt/src/main/java/org/apache/skywalking/oal/v2/generator/OALClassGeneratorV2.java index b50a142462bb..80d13d1424fc 100644 --- a/oap-server/oal-rt/src/main/java/org/apache/skywalking/oal/v2/generator/OALClassGeneratorV2.java +++ b/oap-server/oal-rt/src/main/java/org/apache/skywalking/oal/v2/generator/OALClassGeneratorV2.java @@ -22,6 +22,9 @@ import freemarker.template.Version; import java.io.DataOutputStream; import java.io.File; +import java.nio.charset.StandardCharsets; +import java.io.Writer; +import java.io.OutputStreamWriter; import java.io.FileOutputStream; import java.io.IOException; import java.io.StringWriter; @@ -279,7 +282,6 @@ private Class generateMetricsClass(CodeGenModel model) throws OALCompileExceptio final String body = methodEntity.toString(); javassist.CtMethod m = CtNewMethod.make(body, metricsClass); metricsClass.addMethod(m); - addLineNumberTable(m, 1); sourceMethods.append(" ").append(body.replace("\n", "\n ")).append("\n\n"); } catch (Exception e) { log.error("Can't generate method " + method + " for " + className + ".", e); @@ -300,7 +302,12 @@ private Class generateMetricsClass(CodeGenModel model) throws OALCompileExceptio annotationsAttribute.addAnnotation(streamAnnotation); metricsClassClassFile.addAttribute(annotationsAttribute); - setSourceFile(metricsClass, formatSourceFileName(model, "Metrics")); + // Must equal the .java written by writeSourceFile: a SourceFile naming a file that + // does not exist reads as correct to an IDE right up until it fails to open it. + // No LineNumberTable is attached to any generated method: it numbered bytecode + // boundaries 1, 2, 3 -- statement ORDINALS, not lines in any file. Same policy as LAL. + setSourceFile(metricsClass, + sourceFileFor(metricsClass, model.getMetricDefinition().getLocation())); Class targetClass; try { @@ -368,14 +375,22 @@ private void generateMetricsBuilderClass(CodeGenModel model) throws OALCompileEx .process(model, methodEntity); javassist.CtMethod m = CtNewMethod.make(methodEntity.toString(), metricsBuilderClass); metricsBuilderClass.addMethod(m); - addLineNumberTable(m, 1); } catch (Exception e) { log.error("Can't generate method " + method + " for " + className + ".", e); throw new OALCompileException(e.getMessage(), e); } } - setSourceFile(metricsBuilderClass, formatSourceFileName(model, "MetricsBuilder")); + // No sidecar is written for the builder (writeGeneratedFile below has no + // writeGeneratedSourceFile beside it), so NO value here can resolve to a file. Given that, + // the DSL provenance is strictly more useful than the class's own name: it at least tells + // a reader which OAL rule produced the frame. Switch this to the plain file name only + // together with a builder sidecar that makes it resolve. + // The builder writes no sidecar in ANY mode (writeGeneratedFile below has no + // writeGeneratedSourceFile beside it), so a plain file name would never resolve. Keep the + // provenance unconditionally -- it is the only thing that identifies the rule. + setSourceFile(metricsBuilderClass, + provenanceOf(metricsBuilderClass, model.getMetricDefinition().getLocation())); try { metricsBuilderClass.toClass(MetricBuilderClassPackageHolder.class); @@ -517,7 +532,6 @@ private Class generateDispatcherClass(String scopeName, DispatcherContextV2 disp final String body = methodEntity.toString(); javassist.CtMethod m = CtNewMethod.make(body, dispatcherClass); dispatcherClass.addMethod(m); - addLineNumberTable(m, 1); dispatcherSourceMethods.append(" ").append(body.replace("\n", "\n ")).append("\n\n"); } catch (Exception e) { log.error("Can't generate method do" + metric.getMetricsName() + " for " + className + ".", e); @@ -533,25 +547,19 @@ private Class generateDispatcherClass(String scopeName, DispatcherContextV2 disp final String body = methodEntity.toString(); javassist.CtMethod m = CtNewMethod.make(body, dispatcherClass); dispatcherClass.addMethod(m); - addLineNumberTable(m, 1); dispatcherSourceMethods.append(" ").append(body.replace("\n", "\n ")).append("\n\n"); } catch (Exception e) { log.error("Can't generate method dispatch for " + className + ".", e); throw new OALCompileException(e.getMessage(), e); } - // Use first metric's location for dispatcher SourceFile - if (!dispatcherContext.getMetrics().isEmpty()) { - final CodeGenModel first = dispatcherContext.getMetrics().get(0); - final org.apache.skywalking.oal.v2.model.SourceLocation loc = - first.getMetricDefinition().getLocation(); - final String dispatcherFile = scopeName + "Dispatcher.java"; - if (loc != null && loc != org.apache.skywalking.oal.v2.model.SourceLocation.UNKNOWN) { - setSourceFile(dispatcherClass, "(" + loc.getFileName() + ")" + dispatcherFile); - } else { - setSourceFile(dispatcherClass, dispatcherFile); - } - } + // Must equal the .java written by writeGeneratedSourceFile below. The OAL source location + // used to select this name; it no longer can, because the attribute has to address a file + // that exists rather than describe where the rule came from. + // A dispatcher is shared by every metric of one scope, so no single metric's LINE is + // true for it. Name the .oal file without a line rather than borrow the first metric's, + // which would misreport every other metric routed through this class. + setSourceFile(dispatcherClass, sourceFileFor(dispatcherClass, null)); Class targetClass; try { @@ -631,21 +639,6 @@ public void prepareRTTempFolder() { } } - /** - * Builds the SourceFile name for a generated metrics/builder class. - * Format: {@code (core.oal:20)ServiceRespTime.java} when location is known, - * or {@code ServiceRespTime.java} as fallback. - */ - private String formatSourceFileName(final CodeGenModel model, final String classSuffix) { - final String classFile = model.getMetricsName() + classSuffix + ".java"; - final org.apache.skywalking.oal.v2.model.SourceLocation loc = - model.getMetricDefinition().getLocation(); - if (loc != null && loc != org.apache.skywalking.oal.v2.model.SourceLocation.UNKNOWN) { - return "(" + loc.getFileName() + ":" + loc.getLine() + ")" + classFile; - } - return classFile; - } - /** * Sets the {@code SourceFile} attribute of the class to the given name. */ @@ -663,10 +656,6 @@ private static void setSourceFile(final CtClass ctClass, final String name) { } } - /** - * Adds a {@code LineNumberTable} attribute by scanning bytecode for - * store instructions to local variable slots ≥ {@code firstResultSlot}. - */ /** * Escape a string for embedding inside a Java source-string literal — same shape as * the helpers in MAL / LAL codegen but kept local so this generator stays @@ -683,58 +672,43 @@ private static String escapeJavaLiteral(final String s) { .replace("\t", "\\t"); } - private void addLineNumberTable(final javassist.CtMethod method, - final int firstResultSlot) { - try { - final javassist.bytecode.MethodInfo mi = method.getMethodInfo(); - final javassist.bytecode.CodeAttribute code = mi.getCodeAttribute(); - if (code == null) { - return; - } - final ArrayList entries = new ArrayList<>(); - int line = 1; - boolean nextIsNewLine = true; - - final javassist.bytecode.CodeIterator ci = code.iterator(); - while (ci.hasNext()) { - final int pc = ci.next(); - if (nextIsNewLine) { - entries.add(new int[]{pc, line++}); - nextIsNewLine = false; - } - final int op = ci.byteAt(pc) & 0xFF; - int slot = -1; - if (op >= 59 && op <= 78) { - slot = (op - 59) % 4; - } else if (op >= 54 && op <= 58) { - slot = ci.byteAt(pc + 1) & 0xFF; - } - if (slot >= firstResultSlot) { - nextIsNewLine = true; - } - } - - if (entries.isEmpty()) { - return; - } + /** + * {@code SourceFile} for a generated OAL class. + * + *

Conditional because OAL differs from MAL and LAL: their class NAMES embed the rule's line + * ({@code vm_L38_cpu}), so dropping the parenthesised provenance costs nothing. An OAL class is + * named {@code ServiceRespTimeMetrics} and carries no line anywhere else, so the attribute is + * the only carrier. The sidecar exists only under SW_DYNAMIC_CLASS_ENGINE_DEBUG, so: name the + * real file when one is written, and keep the DSL location when none is. + * + * @param ctClass the generated class + * @param loc the originating OAL location, may be null or UNKNOWN + * @return the attribute value + */ + private String sourceFileFor(final CtClass ctClass, + final org.apache.skywalking.oal.v2.model.SourceLocation loc) { + if (openEngineDebug) { + return ctClass.getSimpleName() + ".java"; + } + return provenanceOf(ctClass, loc); + } - final javassist.bytecode.ConstPool cp = mi.getConstPool(); - final byte[] info = new byte[2 + entries.size() * 4]; - info[0] = (byte) (entries.size() >> 8); - info[1] = (byte) entries.size(); - for (int i = 0; i < entries.size(); i++) { - final int off = 2 + i * 4; - info[off] = (byte) (entries.get(i)[0] >> 8); - info[off + 1] = (byte) entries.get(i)[0]; - info[off + 2] = (byte) (entries.get(i)[1] >> 8); - info[off + 3] = (byte) entries.get(i)[1]; - } - code.getAttributes().add( - new javassist.bytecode.AttributeInfo(cp, "LineNumberTable", info)); - } catch (Exception e) { - log.warn("Failed to add LineNumberTable: {}", e.getMessage()); + /** + * Provenance form, for classes that never get a sidecar in any mode. + * + * @param ctClass the generated class + * @param loc originating OAL location; when null only the file is named, which is correct + * for a class shared by several rules + * @return the attribute value + */ + private static String provenanceOf(final CtClass ctClass, + final org.apache.skywalking.oal.v2.model.SourceLocation loc) { + final String file = ctClass.getSimpleName() + ".java"; + if (loc == null || loc == org.apache.skywalking.oal.v2.model.SourceLocation.UNKNOWN) { + return file; } + return "(" + loc.getFileName() + ":" + loc.getLine() + ")" + file; } private void writeGeneratedFile(CtClass ctClass, String type) throws OALCompileException { @@ -792,8 +766,10 @@ private void writeGeneratedSourceFile(final CtClass ctClass, final String type, folder.mkdirs(); } final File file = new File(folder, ctClass.getSimpleName() + ".java"); - try (java.io.FileWriter w = new java.io.FileWriter(file)) { - w.write("// Synthetic source — Javassist compile input for "); + // UTF-8 explicitly, NOT the platform default: readers open these as UTF-8. + try (Writer w = new OutputStreamWriter( + new FileOutputStream(file), StandardCharsets.UTF_8)) { + w.write("// Synthetic source - Javassist compile input for "); w.write(ctClass.getSimpleName()); w.write("\n// Written when SW_DYNAMIC_CLASS_ENGINE_DEBUG is on; used by IDE\n"); w.write("// source-attach to render the bytecode without FernFlower.\n\n"); diff --git a/oap-server/server-admin/runtime-rule/src/main/java/org/apache/skywalking/oap/server/receiver/runtimerule/apply/MalFileApplier.java b/oap-server/server-admin/runtime-rule/src/main/java/org/apache/skywalking/oap/server/receiver/runtimerule/apply/MalFileApplier.java index 1761e913b628..e0b969b3fa29 100644 --- a/oap-server/server-admin/runtime-rule/src/main/java/org/apache/skywalking/oap/server/receiver/runtimerule/apply/MalFileApplier.java +++ b/oap-server/server-admin/runtime-rule/src/main/java/org/apache/skywalking/oap/server/receiver/runtimerule/apply/MalFileApplier.java @@ -33,6 +33,7 @@ import org.apache.skywalking.oap.meter.analyzer.v2.MetricConvert; import org.apache.skywalking.oap.meter.analyzer.v2.prometheus.rule.MetricsRule; import org.apache.skywalking.oap.meter.analyzer.v2.prometheus.rule.Rule; +import org.apache.skywalking.oap.meter.analyzer.v2.prometheus.rule.RuleSourceLines; import org.apache.skywalking.oap.server.core.CoreModule; import org.apache.skywalking.oap.server.core.analysis.LayerDefinition; import org.apache.skywalking.oap.server.core.analysis.meter.MeterSystem; @@ -279,6 +280,9 @@ private Rule parse(final String yamlContent, final String sourceName) throws App if (rule.getName() == null || rule.getName().isEmpty()) { rule.setName(sourceName); } + // Stamp YAML source anchors from the SAME text we just bound, so a hot-updated rule + // gets the same line provenance a disk-loaded one does. + RuleSourceLines.assign(rule, yamlContent); // layerDefinitions: are now permitted in runtime MAL rules; they're handled by // the layer registry on the apply path below. The rejection that used to live // here was removed when runtime dynamic layers became a first-class feature. diff --git a/oap-server/server-starter/src/test/java/org/apache/skywalking/oap/server/starter/DSLClassGeneratorTest.java b/oap-server/server-starter/src/test/java/org/apache/skywalking/oap/server/starter/DSLClassGeneratorTest.java index 13274bea2a3a..d5edfcbf6181 100644 --- a/oap-server/server-starter/src/test/java/org/apache/skywalking/oap/server/starter/DSLClassGeneratorTest.java +++ b/oap-server/server-starter/src/test/java/org/apache/skywalking/oap/server/starter/DSLClassGeneratorTest.java @@ -40,6 +40,7 @@ import org.apache.skywalking.oal.v2.parser.OALScriptParserV2; import org.apache.skywalking.oap.log.analyzer.v2.compiler.LALClassGenerator; import org.apache.skywalking.oap.log.analyzer.v2.spi.LALSourceTypeProvider; +import org.apache.skywalking.oap.meter.analyzer.v2.MalYamlLineIndex; import org.apache.skywalking.oap.meter.analyzer.v2.compiler.MALClassGenerator; import org.apache.skywalking.oap.server.core.analysis.Layer; import org.apache.skywalking.oap.server.core.analysis.SourceDecoratorManager; @@ -244,6 +245,12 @@ private static int[] compileMALFile(final MALClassGenerator generator, final String relPath = baseDir.toPath().relativize(yamlFile.toPath()).toString(); final String sourceName = relPath.substring(0, relPath.lastIndexOf('.')); final String yamlSource = yamlFile.getName(); + // Resolve real YAML lines once per file: the offline generator must stamp the same + // coordinates production does, or the artifacts it produces are labelled differently + // from the ones the OAP writes at runtime. + final MalYamlLineIndex lineIndex = MalYamlLineIndex.index( + new String(java.nio.file.Files.readAllBytes(yamlFile.toPath()), + java.nio.charset.StandardCharsets.UTF_8)); final String expPrefix = (String) config.get("expPrefix"); final String expSuffix = (String) config.get("expSuffix"); @@ -254,7 +261,7 @@ private static int[] compileMALFile(final MALClassGenerator generator, if (filterText != null && !filterText.trim().isEmpty()) { try { generator.setClassNameHint("filter"); - generator.setYamlSource(yamlSource); + generator.setYamlSource(yamlSource + ":" + lineIndex.getFilterLine()); generator.compileFilter(filterText); filters++; } catch (Exception e) { @@ -290,7 +297,10 @@ private static int[] compileMALFile(final MALClassGenerator generator, try { generator.setClassNameHint(ruleName); - generator.setYamlSource(yamlSource + ":" + i); + // The rule's REAL line, not its index in the list. They agree only by + // accident, and index 0 would render as the unresolved marker. + generator.setYamlSource( + yamlSource + ":" + lineIndex.rule(i).getEntryLine()); generator.compile(metricName, fullExp); expressions++; } catch (Exception e) {