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) {