Skip to content

Commit b7f4b8e

Browse files
wu-shengclaude
wu-sheng
and
claude
committed
Fix review comments from copilot
- Fix resource leak: use try-with-resources for Reader in OALEngineV2 - Fix resource leak: use try-with-resources for FileReader in tests - Fix string comparison: use isEmpty() instead of != "" in deserialize.ftl - Fix doc: OALEngineV2 implements OALEngine (not extends OALKernel) - Fix doc: remove duplicate "Type Safety" section in V2_IMPLEMENTATION_SUMMARY.md - Fix doc: update javadoc to reference DefaultMetricsFunctionRegistry.create() - Add comment explaining method overload design in FilterExpression.Builder Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 8316e6c commit b7f4b8e

File tree

7 files changed

+110
-101
lines changed

7 files changed

+110
-101
lines changed

‎oap-server/oal-rt/V2_IMPLEMENTATION_SUMMARY.md‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ All model classes are immutable, type-safe, and use builder pattern:
8585
### 5. V2 Engine ✅
8686
**Location**: `org.apache.skywalking.oal.v2`
8787

88-
- **OALEngineV2**: Main V2 engine (extends OALKernel)
88+
- **OALEngineV2**: Main V2 engine (implements OALEngine)
8989
- Uses V2 parser for parsing
9090
- Uses V2 enricher for metadata extraction
9191
- Uses V2 generator with V2 templates
@@ -151,8 +151,6 @@ After V1 removal, all code is organized under `org.apache.skywalking.oal.v2`:
151151
## Key Benefits
152152

153153
### 1. Type Safety
154-
155-
### 2. Type Safety
156154
- FilterValue knows its type (NUMBER/STRING/BOOLEAN)
157155
- FunctionArgument distinguishes LITERAL/ATTRIBUTE/EXPRESSION
158156
- Compile-time type checking

‎oap-server/oal-rt/src/main/java/org/apache/skywalking/oal/v2/OALEngineV2.java‎

Lines changed: 28 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -94,39 +94,38 @@ public void start(ClassLoader currentClassLoader) throws ModuleStartException, O
9494
classGeneratorV2.prepareRTTempFolder();
9595
classGeneratorV2.setCurrentClassLoader(currentClassLoader);
9696

97-
// Load OAL script
98-
Reader reader;
99-
try {
100-
reader = ResourceUtils.read(oalDefine.getConfigFile());
97+
// Load OAL script, parse, and generate classes with proper resource management
98+
try (Reader reader = ResourceUtils.read(oalDefine.getConfigFile())) {
99+
// Parse using V2 parser
100+
OALScriptParserV2 v2Parser;
101+
try {
102+
v2Parser = OALScriptParserV2.parse(reader, oalDefine.getConfigFile());
103+
log.info("V2 Parser: Successfully parsed {} metrics", v2Parser.getMetricsCount());
104+
} catch (IOException e) {
105+
throw new ModuleStartException("OAL V2 script parse failure", e);
106+
}
107+
108+
// Enrich V2 models with metadata for code generation
109+
List<CodeGenModel> codeGenModels = enrichMetrics(v2Parser.getMetrics());
110+
log.info("V2 Enricher: Enriched {} metrics with metadata", codeGenModels.size());
111+
112+
// Generate classes using V2 generator
113+
classGeneratorV2.generateClassAtRuntime(
114+
codeGenModels,
115+
v2Parser.getDisabledSources(),
116+
metricsClasses,
117+
dispatcherClasses
118+
);
119+
120+
log.info("OAL Engine V2 started successfully. Generated {} metrics classes, {} dispatcher classes",
121+
metricsClasses.size(),
122+
dispatcherClasses.size()
123+
);
101124
} catch (FileNotFoundException e) {
102125
throw new ModuleStartException("Can't locate " + oalDefine.getConfigFile(), e);
103-
}
104-
105-
// Parse using V2 parser
106-
OALScriptParserV2 v2Parser;
107-
try {
108-
v2Parser = OALScriptParserV2.parse(reader, oalDefine.getConfigFile());
109-
log.info("V2 Parser: Successfully parsed {} metrics", v2Parser.getMetricsCount());
110126
} catch (IOException e) {
111-
throw new ModuleStartException("OAL V2 script parse failure", e);
127+
throw new ModuleStartException("OAL V2 script I/O failure", e);
112128
}
113-
114-
// Enrich V2 models with metadata for code generation
115-
List<CodeGenModel> codeGenModels = enrichMetrics(v2Parser.getMetrics());
116-
log.info("V2 Enricher: Enriched {} metrics with metadata", codeGenModels.size());
117-
118-
// Generate classes using V2 generator
119-
classGeneratorV2.generateClassAtRuntime(
120-
codeGenModels,
121-
v2Parser.getDisabledSources(),
122-
metricsClasses,
123-
dispatcherClasses
124-
);
125-
126-
log.info("OAL Engine V2 started successfully. Generated {} metrics classes, {} dispatcher classes",
127-
metricsClasses.size(),
128-
dispatcherClasses.size()
129-
);
130129
}
131130

132131
@Override

‎oap-server/oal-rt/src/main/java/org/apache/skywalking/oal/v2/model/FilterExpression.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,10 @@ public Builder value(FilterValue value) {
132132

133133
/**
134134
* Set value with auto-type detection.
135+
*
136+
* Note: This method is intentionally overloaded with value(FilterValue).
137+
* Java dispatches based on compile-time type, but both methods handle
138+
* FilterValue correctly (this method checks instanceof FilterValue).
135139
*/
136140
public Builder value(Object value) {
137141
this.value = convertToFilterValue(value);

‎oap-server/oal-rt/src/main/java/org/apache/skywalking/oal/v2/registry/MetricsFunctionRegistry.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
*
3434
* Example usage:
3535
* <pre>
36-
* MetricsFunctionRegistry registry = MetricsFunctionRegistryFactory.createDefault();
36+
* MetricsFunctionRegistry registry = DefaultMetricsFunctionRegistry.create();
3737
*
3838
* Optional&lt;MetricsFunctionDescriptor&gt; longAvg = registry.findFunction("longAvg");
3939
* if (longAvg.isPresent()) {

‎oap-server/oal-rt/src/main/resources/code-templates-v2/metrics/deserialize.ftl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
public void deserialize(org.apache.skywalking.oap.server.core.remote.grpc.proto.RemoteData remoteData) {
22
<#if serializeFields.stringFields?? && serializeFields.stringFields?size gt 0>
33
<#list serializeFields.stringFields as field>
4-
if (remoteData.getDataStrings(${field_index}) != "") {
4+
if (!remoteData.getDataStrings(${field_index}).isEmpty()) {
55
${field.setter}(remoteData.getDataStrings(${field_index}));
66
}
77
</#list>

‎oap-server/oal-rt/src/test/java/org/apache/skywalking/oal/v2/generator/RuntimeOALGenerationTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,9 +242,9 @@ public void testGenerateAllRuntimeOALScripts() throws Exception {
242242
assertNotNull(oalFile, "OAL file not found: " + define.getConfigFile() +
243243
". Tried paths: " + String.join(", ", POSSIBLE_PATHS));
244244

245-
try {
245+
try (FileReader reader = new FileReader(oalFile)) {
246246
// Parse OAL script
247-
OALScriptParserV2 parser = OALScriptParserV2.parse(new FileReader(oalFile), define.getConfigFile());
247+
OALScriptParserV2 parser = OALScriptParserV2.parse(reader, define.getConfigFile());
248248
List<MetricDefinition> metrics = parser.getMetrics();
249249
List<String> disabledSources = parser.getDisabledSources();
250250
totalMetrics += metrics.size();

‎oap-server/oal-rt/src/test/java/org/apache/skywalking/oal/v2/parser/RealOALScriptsTest.java‎

Lines changed: 73 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -74,35 +74,37 @@ public void testParseCoreOAL() throws IOException {
7474
File oalFile = new File(oalDir, "core.oal");
7575
assertTrue(oalFile.exists(), "core.oal not found at: " + oalFile.getAbsolutePath());
7676

77-
OALScriptParserV2 parser = OALScriptParserV2.parse(new FileReader(oalFile), "core.oal");
78-
79-
assertNotNull(parser);
80-
assertTrue(parser.hasMetrics(), "core.oal should have metrics");
81-
82-
log.info("✅ Parsed core.oal: {} metrics", parser.getMetricsCount());
83-
84-
// Verify some expected metrics exist
85-
boolean foundServiceRespTime = false;
86-
boolean foundServiceSla = false;
87-
boolean foundServiceCpm = false;
88-
89-
for (MetricDefinition metric : parser.getMetrics()) {
90-
String name = metric.getName();
91-
if (name.equals("service_resp_time")) {
92-
foundServiceRespTime = true;
93-
log.info(" - Found: service_resp_time (source: {}, function: {})",
94-
metric.getSource().getName(),
95-
metric.getAggregationFunction().getName());
96-
} else if (name.equals("service_sla")) {
97-
foundServiceSla = true;
98-
} else if (name.equals("service_cpm")) {
99-
foundServiceCpm = true;
77+
try (FileReader reader = new FileReader(oalFile)) {
78+
OALScriptParserV2 parser = OALScriptParserV2.parse(reader, "core.oal");
79+
80+
assertNotNull(parser);
81+
assertTrue(parser.hasMetrics(), "core.oal should have metrics");
82+
83+
log.info("Parsed core.oal: {} metrics", parser.getMetricsCount());
84+
85+
// Verify some expected metrics exist
86+
boolean foundServiceRespTime = false;
87+
boolean foundServiceSla = false;
88+
boolean foundServiceCpm = false;
89+
90+
for (MetricDefinition metric : parser.getMetrics()) {
91+
String name = metric.getName();
92+
if (name.equals("service_resp_time")) {
93+
foundServiceRespTime = true;
94+
log.info(" - Found: service_resp_time (source: {}, function: {})",
95+
metric.getSource().getName(),
96+
metric.getAggregationFunction().getName());
97+
} else if (name.equals("service_sla")) {
98+
foundServiceSla = true;
99+
} else if (name.equals("service_cpm")) {
100+
foundServiceCpm = true;
101+
}
100102
}
101-
}
102103

103-
assertTrue(foundServiceRespTime, "Should find service_resp_time");
104-
assertTrue(foundServiceSla, "Should find service_sla");
105-
assertTrue(foundServiceCpm, "Should find service_cpm");
104+
assertTrue(foundServiceRespTime, "Should find service_resp_time");
105+
assertTrue(foundServiceSla, "Should find service_sla");
106+
assertTrue(foundServiceCpm, "Should find service_cpm");
107+
}
106108
}
107109

108110
/**
@@ -130,24 +132,24 @@ public void testParseAllOALFiles() throws IOException {
130132
File oalFile = new File(oalDir, fileName);
131133
assertTrue(oalFile.exists(), fileName + " not found at: " + oalFile.getAbsolutePath());
132134

133-
try {
134-
OALScriptParserV2 parser = OALScriptParserV2.parse(new FileReader(oalFile), fileName);
135+
try (FileReader reader = new FileReader(oalFile)) {
136+
OALScriptParserV2 parser = OALScriptParserV2.parse(reader, fileName);
135137
int metricsCount = parser.getMetricsCount();
136138
totalMetrics += metricsCount;
137139
successCount++;
138140

139-
log.info("✅ Parsed {}: {} metrics, {} disabled sources",
141+
log.info("Parsed {}: {} metrics, {} disabled sources",
140142
fileName,
141143
metricsCount,
142144
parser.getDisabledSources().size());
143145

144146
} catch (Exception e) {
145-
log.error("❌ Failed to parse {}: {}", fileName, e.getMessage(), e);
147+
log.error("Failed to parse {}: {}", fileName, e.getMessage(), e);
146148
throw e;
147149
}
148150
}
149151

150-
log.info("✅ Successfully parsed {}/{} OAL files, total {} metrics",
152+
log.info("Successfully parsed {}/{} OAL files, total {} metrics",
151153
successCount, oalFiles.length, totalMetrics);
152154

153155
assertTrue(successCount >= 5, "Should successfully parse at least 5 OAL files");
@@ -167,12 +169,14 @@ public void testCommentsInRealOAL() throws IOException {
167169
}
168170

169171
// core.oal has line comments like: // Multiple values including p50, p75, p90, p95, p99
170-
OALScriptParserV2 parser = OALScriptParserV2.parse(new FileReader(oalFile), "core.oal");
172+
try (FileReader reader = new FileReader(oalFile)) {
173+
OALScriptParserV2 parser = OALScriptParserV2.parse(reader, "core.oal");
171174

172-
assertNotNull(parser);
173-
assertTrue(parser.hasMetrics());
175+
assertNotNull(parser);
176+
assertTrue(parser.hasMetrics());
174177

175-
log.info("✅ V2 parser correctly handles comments in OAL scripts");
178+
log.info("V2 parser correctly handles comments in OAL scripts");
179+
}
176180
}
177181

178182
/**
@@ -187,22 +191,24 @@ public void testDecoratorsInRealOAL() throws IOException {
187191
return;
188192
}
189193

190-
OALScriptParserV2 parser = OALScriptParserV2.parse(new FileReader(oalFile), "core.oal");
191-
192-
// core.oal has: service_resp_time = from(Service.latency).longAvg().decorator("ServiceDecorator");
193-
boolean foundDecorator = false;
194-
for (MetricDefinition metric : parser.getMetrics()) {
195-
if (metric.getDecorator().isPresent()) {
196-
foundDecorator = true;
197-
log.info(" - Found decorator: {} in metric: {}",
198-
metric.getDecorator().get(),
199-
metric.getName());
200-
break;
194+
try (FileReader reader = new FileReader(oalFile)) {
195+
OALScriptParserV2 parser = OALScriptParserV2.parse(reader, "core.oal");
196+
197+
// core.oal has: service_resp_time = from(Service.latency).longAvg().decorator("ServiceDecorator");
198+
boolean foundDecorator = false;
199+
for (MetricDefinition metric : parser.getMetrics()) {
200+
if (metric.getDecorator().isPresent()) {
201+
foundDecorator = true;
202+
log.info(" - Found decorator: {} in metric: {}",
203+
metric.getDecorator().get(),
204+
metric.getName());
205+
break;
206+
}
201207
}
202-
}
203208

204-
assertTrue(foundDecorator, "Should find at least one metric with decorator");
205-
log.info("✅ V2 parser correctly handles decorators");
209+
assertTrue(foundDecorator, "Should find at least one metric with decorator");
210+
log.info("V2 parser correctly handles decorators");
211+
}
206212
}
207213

208214
/**
@@ -217,21 +223,23 @@ public void testComplexFiltersInRealOAL() throws IOException {
217223
return;
218224
}
219225

220-
OALScriptParserV2 parser = OALScriptParserV2.parse(new FileReader(oalFile), "core.oal");
221-
222-
// core.oal has: from(ServiceRelation.*).filter(detectPoint == DetectPoint.CLIENT)
223-
boolean foundComplexFilter = false;
224-
for (MetricDefinition metric : parser.getMetrics()) {
225-
if (!metric.getFilters().isEmpty()) {
226-
foundComplexFilter = true;
227-
log.info(" - Found filter in metric: {} ({} filters)",
228-
metric.getName(),
229-
metric.getFilters().size());
230-
break;
226+
try (FileReader reader = new FileReader(oalFile)) {
227+
OALScriptParserV2 parser = OALScriptParserV2.parse(reader, "core.oal");
228+
229+
// core.oal has: from(ServiceRelation.*).filter(detectPoint == DetectPoint.CLIENT)
230+
boolean foundComplexFilter = false;
231+
for (MetricDefinition metric : parser.getMetrics()) {
232+
if (!metric.getFilters().isEmpty()) {
233+
foundComplexFilter = true;
234+
log.info(" - Found filter in metric: {} ({} filters)",
235+
metric.getName(),
236+
metric.getFilters().size());
237+
break;
238+
}
231239
}
232-
}
233240

234-
assertTrue(foundComplexFilter, "Should find metrics with filters");
235-
log.info("✅ V2 parser correctly handles complex filters");
241+
assertTrue(foundComplexFilter, "Should find metrics with filters");
242+
log.info("V2 parser correctly handles complex filters");
243+
}
236244
}
237-
}
245+
}

0 commit comments

Comments
 (0)