Skip to content

Commit 432d55f

Browse files
authored
Fix MQE top_n raising a raw storage IO exception for an invalid attribute (#13940)
An MQE top_n(metric, N, order, attrX='value') whose attribute is not a queryable column of the metric reached the storage engine with a tag the measure does not define and failed there, surfacing the opaque "Internal IO exception, query metrics error.". Attribute columns (attr0..attrN) exist only on decorated metrics (service_*/endpoint_*/kubernetes_service_*, set to the layer via OAL .decorator()) and the MAL meter base; relations and database/cache/mq access carry none. MQEVisitor now validates each attribute key against the metric's registered queryable columns before the storage call and raises IllegalExpressionException (naming the attribute and metric), which the existing catch turns into a descriptive MQE error instead of the opaque storage exception.
1 parent 585d195 commit 432d55f

3 files changed

Lines changed: 191 additions & 1 deletion

File tree

docs/en/changes/changes.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,7 @@
332332
* Fix: the v2 MAL compiler now resolves custom layers referenced as `Layer.NAME` in an expression. A custom layer declared through a `layerDefinitions:` block (or `layer-extensions.yml` / the `LayerExtension` SPI) has no generated `Layer.*` static field, so `service(['svc'], Layer.IOT_FLEET)` previously failed code generation because `Layer` has no `IOT_FLEET` field. The compiler now lowers every `Layer.NAME` static-field reference to a runtime `Layer.nameOf("NAME")` registry lookup, so a custom layer can be referenced exactly like a built-in one (`Layer.GENERAL`). For a built-in layer this is equivalent, because `Layer.nameOf("GENERAL")` returns the same instance as the `Layer.GENERAL` field. The lowering is scoped to `Layer` only; the other MAL enum types (`DetectPoint`, `DownsamplingType`, etc.) are real Java enums and keep their direct static-field reference.
333333
* Fix Envoy ALS rendering for the LAL live-debugger and the persisted log `content`: an Istio metadata-exchange peer in `common_properties.filter_state_objects` (legacy Wasm `wasm.*_peer` = `Any{BytesValue}` wrapping a FlatBuffer, or modern `*_peer` = `Any{Struct}`) is now decoded into the readable peer metadata (pod / namespace / labels) instead of an opaque `jsonformat-failed` envelope or base64. The serialization is hardened so a single un-printable field can no longer blank the whole entry — the `LalPayloadDebugDump` printer carries a well-known-type `TypeRegistry` and sanitizes every value `JsonFormat` would reject (an unresolvable, no-slash, or corrupt-bytes `Any` degrades to an `@unresolved` placeholder; a non-finite `Value` double `NaN`/`Infinity` is rendered as a string), keeping the rest of the entry readable. Because the LAL output builder's `bindInput` runs eagerly before the debug capture, this also stops an unregistered `filter_state_objects` type from throwing and aborting the whole rule (dropping the mesh log). Decoding is wired through a new `LalInputDebugRenderer` SPI (`EnvoyAlsHttpDebugRenderer` / `EnvoyAlsTcpDebugRenderer`) so `log-analyzer` reaches the receiver-side decoders without depending on the Envoy receiver, and covers both HTTP and TCP access logs.
334334
* Surface the effective BanyanDB configuration (`bydb.yml` / `bydb-topn.yml`) in the `/debugging/config/dump` admin API. Because the BanyanDB config moved to a separate file in 10.2.0, a BanyanDB deployment previously showed an empty `storage.banyandb` block in the dump; its post-environment-resolution values are now merged into the same response under `storage.banyandb.*` (TopN rules under `storage.banyandb.topN.*`), masked by the same secret-keyword list, via a generic `ConfigDumpExtension` SPI on `ServerStatusService` that any module loading config from a secondary file can implement.
335+
* Fix: an MQE `top_n(metric, N, order, attrX='value')` query whose attribute is not a column of the target metric now returns a descriptive MQE error instead of a raw storage `IOException` surfaced as `Internal IO exception, query metrics error.`. Attribute columns (`attr0..attrN`) exist only on decorated metrics (`service_*` / `endpoint_*` / `kubernetes_service_*`, set to the layer name via OAL `.decorator(...)`) and the MAL meter base; metrics such as relations or database / cache / mq access carry none, so passing an attribute condition previously reached the storage engine with a tag it does not define and failed there. `MQEVisitor` now validates each attribute key against the metric's registered queryable columns before the storage call and raises `IllegalExpressionException` (naming the attribute and the metric) when it is absent.
335336

336337
#### UI
337338
* Add Airflow layer dashboards and menu i18n under Workflow Scheduler in Horizon UI (SWIP-7).

oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/mqe/rt/MQEVisitor.java

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
import java.util.List;
2525
import java.util.Map;
2626
import java.util.Optional;
27+
import java.util.Set;
28+
import java.util.stream.Collectors;
2729
import lombok.extern.slf4j.Slf4j;
2830
import org.apache.skywalking.mqe.rt.exception.IllegalExpressionException;
2931
import org.apache.skywalking.mqe.rt.grammar.MQEParser;
@@ -52,6 +54,9 @@
5254
import org.apache.skywalking.oap.server.core.query.type.debugging.DebuggingTraceContext;
5355
import org.apache.skywalking.oap.server.core.storage.annotation.Column;
5456
import org.apache.skywalking.oap.server.core.storage.annotation.ValueColumnMetadata;
57+
import org.apache.skywalking.oap.server.core.storage.model.IModelManager;
58+
import org.apache.skywalking.oap.server.core.storage.model.Model;
59+
import org.apache.skywalking.oap.server.core.storage.model.ModelColumn;
5560
import org.apache.skywalking.oap.server.library.module.ModuleManager;
5661
import org.apache.skywalking.mqe.rt.MQEVisitorBase;
5762
import org.apache.skywalking.oap.server.core.query.mqe.ExpressionResult;
@@ -69,6 +74,7 @@ public class MQEVisitor extends MQEVisitorBase {
6974
private MetricsQueryService metricsQueryService;
7075
private AggregationQueryService aggregationQueryService;
7176
private RecordQueryService recordQueryService;
77+
private IModelManager modelManager;
7278

7379
public MQEVisitor(final ModuleManager moduleManager,
7480
final Entity entity,
@@ -97,6 +103,15 @@ private AggregationQueryService getAggregationQueryService() {
97103
return aggregationQueryService;
98104
}
99105

106+
private IModelManager getModelManager() {
107+
if (modelManager == null) {
108+
this.modelManager = moduleManager.find(CoreModule.NAME)
109+
.provider()
110+
.getService(IModelManager.class);
111+
}
112+
return modelManager;
113+
}
114+
100115
private RecordQueryService getRecordQueryService() {
101116
if (recordQueryService == null) {
102117
this.recordQueryService = moduleManager.find(CoreModule.NAME)
@@ -220,11 +235,49 @@ public ExpressionResult visitMetric(MQEParser.MetricContext ctx) {
220235
}
221236
}
222237

238+
/**
239+
* Reject a top_n attribute condition whose key is not a queryable column of the metric, before the
240+
* request reaches storage. Attribute columns (attr0..attrN) exist only on decorated OAL metrics and
241+
* the MAL meter base; metrics such as relations or database/cache/mq access carry none, so passing the
242+
* condition to the storage engine surfaces a raw backend exception instead of a query result. Failing
243+
* here turns that into a descriptive MQE error via {@link #getErrorResult(String)}.
244+
*
245+
* @param metricName the queried metric name
246+
* @param attrConditions the attribute conditions parsed from the top_n function
247+
* @throws IllegalExpressionException if an attribute key is not a queryable column of the metric
248+
*/
249+
void checkAttributes(String metricName,
250+
List<AttrCondition> attrConditions) throws IllegalExpressionException {
251+
if (attrConditions == null || attrConditions.isEmpty()) {
252+
return;
253+
}
254+
Optional<Model> model = getModelManager().allModels().stream()
255+
.filter(m -> m.getName().equals(metricName))
256+
.findFirst();
257+
if (model.isEmpty()) {
258+
// The metric existence is already validated via ValueColumnMetadata; a missing model here is
259+
// unexpected, so leave the condition untouched rather than reject a possibly-valid query.
260+
return;
261+
}
262+
Set<String> queryableColumns = model.get().getColumns().stream()
263+
.filter(ModelColumn::shouldIndex)
264+
.map(column -> column.getColumnName().getName())
265+
.collect(Collectors.toSet());
266+
for (AttrCondition attr : attrConditions) {
267+
if (!queryableColumns.contains(attr.getKey())) {
268+
throw new IllegalExpressionException(
269+
"The attribute [" + attr.getKey() + "] is not a queryable column of metric [" + metricName
270+
+ "], so it can not be used as a top_n condition.");
271+
}
272+
}
273+
}
274+
223275
private void querySortMetrics(String metricName,
224276
int topN,
225277
Order order,
226278
List<AttrCondition> attrConditions,
227-
ExpressionResult result) throws IOException {
279+
ExpressionResult result) throws IOException, IllegalExpressionException {
280+
checkAttributes(metricName, attrConditions);
228281
TopNCondition topNCondition = new TopNCondition();
229282
topNCondition.setName(metricName);
230283
topNCondition.setTopN(topN);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*
17+
*/
18+
19+
package org.apache.skywalking.oap.query.graphql.mqe.rt;
20+
21+
import java.util.ArrayList;
22+
import java.util.Arrays;
23+
import java.util.Collections;
24+
import java.util.List;
25+
import org.apache.skywalking.mqe.rt.exception.IllegalExpressionException;
26+
import org.apache.skywalking.oap.server.core.CoreModule;
27+
import org.apache.skywalking.oap.server.core.query.enumeration.Step;
28+
import org.apache.skywalking.oap.server.core.query.input.AttrCondition;
29+
import org.apache.skywalking.oap.server.core.query.input.Duration;
30+
import org.apache.skywalking.oap.server.core.query.input.Entity;
31+
import org.apache.skywalking.oap.server.core.storage.model.ColumnName;
32+
import org.apache.skywalking.oap.server.core.storage.model.IModelManager;
33+
import org.apache.skywalking.oap.server.core.storage.model.Model;
34+
import org.apache.skywalking.oap.server.core.storage.model.ModelColumn;
35+
import org.apache.skywalking.oap.server.library.module.ModuleManager;
36+
import org.apache.skywalking.oap.server.library.module.ModuleProviderHolder;
37+
import org.apache.skywalking.oap.server.library.module.ModuleServiceHolder;
38+
import org.junit.jupiter.api.BeforeEach;
39+
import org.junit.jupiter.api.Test;
40+
import org.junit.jupiter.api.extension.ExtendWith;
41+
import org.mockito.Mock;
42+
import org.mockito.junit.jupiter.MockitoExtension;
43+
import org.mockito.junit.jupiter.MockitoSettings;
44+
import org.mockito.quality.Strictness;
45+
46+
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
47+
import static org.junit.jupiter.api.Assertions.assertThrows;
48+
import static org.junit.jupiter.api.Assertions.assertTrue;
49+
import static org.mockito.Mockito.mock;
50+
import static org.mockito.Mockito.when;
51+
52+
/**
53+
* Guards the top_n attribute pre-check in {@link MQEVisitor#checkAttributes}: an attribute condition whose
54+
* key is not a queryable column of the metric must be rejected with a descriptive {@link
55+
* IllegalExpressionException} before the request reaches storage, instead of letting the backend surface a
56+
* raw IO exception. Metrics with no attribute columns (relations, database/cache/mq access) are the case
57+
* that used to fail.
58+
*/
59+
@ExtendWith(MockitoExtension.class)
60+
@MockitoSettings(strictness = Strictness.LENIENT)
61+
public class MQEVisitorAttributeTest {
62+
@Mock
63+
private ModuleManager moduleManager;
64+
@Mock
65+
private IModelManager modelManager;
66+
67+
private MQEVisitor visitor;
68+
69+
@BeforeEach
70+
public void setup() {
71+
final ModuleProviderHolder providerHolder = mock(ModuleProviderHolder.class);
72+
final ModuleServiceHolder serviceHolder = mock(ModuleServiceHolder.class);
73+
when(moduleManager.find(CoreModule.NAME)).thenReturn(providerHolder);
74+
when(providerHolder.provider()).thenReturn(serviceHolder);
75+
when(serviceHolder.getService(IModelManager.class)).thenReturn(modelManager);
76+
// Build the model mocks before stubbing allModels(); nesting when() calls confuses Mockito.
77+
final List<Model> models = Arrays.asList(
78+
// endpoint_cpm is decorated, so its measure carries the attr0 tag.
79+
modelWithColumns("endpoint_cpm", "entity_id", "service_id", "attr0"),
80+
// a relation metric carries no attribute columns at all.
81+
modelWithColumns("service_relation_client_cpm", "entity_id", "source_service_id", "dest_service_id"));
82+
when(modelManager.allModels()).thenReturn(models);
83+
84+
final Duration duration = mock(Duration.class);
85+
when(duration.getStep()).thenReturn(Step.MINUTE);
86+
visitor = new MQEVisitor(moduleManager, mock(Entity.class), duration);
87+
}
88+
89+
@Test
90+
public void rejectsAttributeMissingFromMetricSchema() {
91+
final IllegalExpressionException ex = assertThrows(IllegalExpressionException.class, () ->
92+
visitor.checkAttributes(
93+
"service_relation_client_cpm",
94+
Collections.singletonList(new AttrCondition("attr0", "VIRTUAL_DATABASE", true))));
95+
assertTrue(ex.getMessage().contains("attr0"));
96+
assertTrue(ex.getMessage().contains("service_relation_client_cpm"));
97+
}
98+
99+
@Test
100+
public void rejectsNotEqualsAttributeMissingFromMetricSchema() {
101+
// the same holds for `attr0 != x`, since the check is on the attribute key.
102+
assertThrows(IllegalExpressionException.class, () ->
103+
visitor.checkAttributes(
104+
"service_relation_client_cpm",
105+
Collections.singletonList(new AttrCondition("attr0", "MESH", false))));
106+
}
107+
108+
@Test
109+
public void allowsAttributePresentInMetricSchema() {
110+
assertDoesNotThrow(() ->
111+
visitor.checkAttributes(
112+
"endpoint_cpm",
113+
Collections.singletonList(new AttrCondition("attr0", "GENERAL", true))));
114+
}
115+
116+
@Test
117+
public void allowsEmptyAttributes() {
118+
assertDoesNotThrow(() -> visitor.checkAttributes("service_relation_client_cpm", Collections.emptyList()));
119+
}
120+
121+
private static Model modelWithColumns(String name, String... columnNames) {
122+
final Model model = mock(Model.class);
123+
when(model.getName()).thenReturn(name);
124+
final List<ModelColumn> columns = new ArrayList<>(columnNames.length);
125+
for (final String columnName : columnNames) {
126+
final ColumnName wrappedName = mock(ColumnName.class);
127+
when(wrappedName.getName()).thenReturn(columnName);
128+
final ModelColumn column = mock(ModelColumn.class);
129+
when(column.getColumnName()).thenReturn(wrappedName);
130+
when(column.shouldIndex()).thenReturn(true);
131+
columns.add(column);
132+
}
133+
when(model.getColumns()).thenReturn(columns);
134+
return model;
135+
}
136+
}

0 commit comments

Comments
 (0)