This is an automated email from the ASF dual-hosted git repository. wu-sheng pushed a commit to branch fix/mqe-topn-attribute-validation in repository https://gitbox.apache.org/repos/asf/skywalking.git
commit 81cfe9021c269e30b62bcd61b05d14dd4a05a112 Author: Wu Sheng <[email protected]> AuthorDate: Tue Jul 7 19:48:58 2026 +0800 Fix MQE top_n raising a raw storage IO exception for an invalid attribute 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. --- docs/en/changes/changes.md | 1 + .../oap/query/graphql/mqe/rt/MQEVisitor.java | 55 ++++++++- .../graphql/mqe/rt/MQEVisitorAttributeTest.java | 136 +++++++++++++++++++++ 3 files changed, 191 insertions(+), 1 deletion(-) diff --git a/docs/en/changes/changes.md b/docs/en/changes/changes.md index 950d00b557..15839723ec 100644 --- a/docs/en/changes/changes.md +++ b/docs/en/changes/changes.md @@ -332,6 +332,7 @@ * 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 [...] * 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 [...] * 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 `ConfigDumpExten [...] +* 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 [...] #### UI * Add Airflow layer dashboards and menu i18n under Workflow Scheduler in Horizon UI (SWIP-7). diff --git a/oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/mqe/rt/MQEVisitor.java b/oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/mqe/rt/MQEVisitor.java index 9fedf928fa..e176af6293 100644 --- a/oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/mqe/rt/MQEVisitor.java +++ b/oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/mqe/rt/MQEVisitor.java @@ -24,6 +24,8 @@ import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; import org.apache.skywalking.mqe.rt.exception.IllegalExpressionException; import org.apache.skywalking.mqe.rt.grammar.MQEParser; @@ -52,6 +54,9 @@ import org.apache.skywalking.oap.server.core.query.type.debugging.DebuggingSpan; import org.apache.skywalking.oap.server.core.query.type.debugging.DebuggingTraceContext; import org.apache.skywalking.oap.server.core.storage.annotation.Column; import org.apache.skywalking.oap.server.core.storage.annotation.ValueColumnMetadata; +import org.apache.skywalking.oap.server.core.storage.model.IModelManager; +import org.apache.skywalking.oap.server.core.storage.model.Model; +import org.apache.skywalking.oap.server.core.storage.model.ModelColumn; import org.apache.skywalking.oap.server.library.module.ModuleManager; import org.apache.skywalking.mqe.rt.MQEVisitorBase; import org.apache.skywalking.oap.server.core.query.mqe.ExpressionResult; @@ -69,6 +74,7 @@ public class MQEVisitor extends MQEVisitorBase { private MetricsQueryService metricsQueryService; private AggregationQueryService aggregationQueryService; private RecordQueryService recordQueryService; + private IModelManager modelManager; public MQEVisitor(final ModuleManager moduleManager, final Entity entity, @@ -97,6 +103,15 @@ public class MQEVisitor extends MQEVisitorBase { return aggregationQueryService; } + private IModelManager getModelManager() { + if (modelManager == null) { + this.modelManager = moduleManager.find(CoreModule.NAME) + .provider() + .getService(IModelManager.class); + } + return modelManager; + } + private RecordQueryService getRecordQueryService() { if (recordQueryService == null) { this.recordQueryService = moduleManager.find(CoreModule.NAME) @@ -220,11 +235,49 @@ public class MQEVisitor extends MQEVisitorBase { } } + /** + * Reject a top_n attribute condition whose key is not a queryable column of the metric, before the + * request reaches storage. Attribute columns (attr0..attrN) exist only on decorated OAL metrics and + * the MAL meter base; metrics such as relations or database/cache/mq access carry none, so passing the + * condition to the storage engine surfaces a raw backend exception instead of a query result. Failing + * here turns that into a descriptive MQE error via {@link #getErrorResult(String)}. + * + * @param metricName the queried metric name + * @param attrConditions the attribute conditions parsed from the top_n function + * @throws IllegalExpressionException if an attribute key is not a queryable column of the metric + */ + void checkAttributes(String metricName, + List<AttrCondition> attrConditions) throws IllegalExpressionException { + if (attrConditions == null || attrConditions.isEmpty()) { + return; + } + Optional<Model> model = getModelManager().allModels().stream() + .filter(m -> m.getName().equals(metricName)) + .findFirst(); + if (model.isEmpty()) { + // The metric existence is already validated via ValueColumnMetadata; a missing model here is + // unexpected, so leave the condition untouched rather than reject a possibly-valid query. + return; + } + Set<String> queryableColumns = model.get().getColumns().stream() + .filter(ModelColumn::shouldIndex) + .map(column -> column.getColumnName().getName()) + .collect(Collectors.toSet()); + for (AttrCondition attr : attrConditions) { + if (!queryableColumns.contains(attr.getKey())) { + throw new IllegalExpressionException( + "The attribute [" + attr.getKey() + "] is not a queryable column of metric [" + metricName + + "], so it can not be used as a top_n condition."); + } + } + } + private void querySortMetrics(String metricName, int topN, Order order, List<AttrCondition> attrConditions, - ExpressionResult result) throws IOException { + ExpressionResult result) throws IOException, IllegalExpressionException { + checkAttributes(metricName, attrConditions); TopNCondition topNCondition = new TopNCondition(); topNCondition.setName(metricName); topNCondition.setTopN(topN); diff --git a/oap-server/server-query-plugin/query-graphql-plugin/src/test/java/org/apache/skywalking/oap/query/graphql/mqe/rt/MQEVisitorAttributeTest.java b/oap-server/server-query-plugin/query-graphql-plugin/src/test/java/org/apache/skywalking/oap/query/graphql/mqe/rt/MQEVisitorAttributeTest.java new file mode 100644 index 0000000000..f94b3f4004 --- /dev/null +++ b/oap-server/server-query-plugin/query-graphql-plugin/src/test/java/org/apache/skywalking/oap/query/graphql/mqe/rt/MQEVisitorAttributeTest.java @@ -0,0 +1,136 @@ +/* + * 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.query.graphql.mqe.rt; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import org.apache.skywalking.mqe.rt.exception.IllegalExpressionException; +import org.apache.skywalking.oap.server.core.CoreModule; +import org.apache.skywalking.oap.server.core.query.enumeration.Step; +import org.apache.skywalking.oap.server.core.query.input.AttrCondition; +import org.apache.skywalking.oap.server.core.query.input.Duration; +import org.apache.skywalking.oap.server.core.query.input.Entity; +import org.apache.skywalking.oap.server.core.storage.model.ColumnName; +import org.apache.skywalking.oap.server.core.storage.model.IModelManager; +import org.apache.skywalking.oap.server.core.storage.model.Model; +import org.apache.skywalking.oap.server.core.storage.model.ModelColumn; +import org.apache.skywalking.oap.server.library.module.ModuleManager; +import org.apache.skywalking.oap.server.library.module.ModuleProviderHolder; +import org.apache.skywalking.oap.server.library.module.ModuleServiceHolder; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Guards the top_n attribute pre-check in {@link MQEVisitor#checkAttributes}: an attribute condition whose + * key is not a queryable column of the metric must be rejected with a descriptive {@link + * IllegalExpressionException} before the request reaches storage, instead of letting the backend surface a + * raw IO exception. Metrics with no attribute columns (relations, database/cache/mq access) are the case + * that used to fail. + */ +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +public class MQEVisitorAttributeTest { + @Mock + private ModuleManager moduleManager; + @Mock + private IModelManager modelManager; + + private MQEVisitor visitor; + + @BeforeEach + public void setup() { + final ModuleProviderHolder providerHolder = mock(ModuleProviderHolder.class); + final ModuleServiceHolder serviceHolder = mock(ModuleServiceHolder.class); + when(moduleManager.find(CoreModule.NAME)).thenReturn(providerHolder); + when(providerHolder.provider()).thenReturn(serviceHolder); + when(serviceHolder.getService(IModelManager.class)).thenReturn(modelManager); + // Build the model mocks before stubbing allModels(); nesting when() calls confuses Mockito. + final List<Model> models = Arrays.asList( + // endpoint_cpm is decorated, so its measure carries the attr0 tag. + modelWithColumns("endpoint_cpm", "entity_id", "service_id", "attr0"), + // a relation metric carries no attribute columns at all. + modelWithColumns("service_relation_client_cpm", "entity_id", "source_service_id", "dest_service_id")); + when(modelManager.allModels()).thenReturn(models); + + final Duration duration = mock(Duration.class); + when(duration.getStep()).thenReturn(Step.MINUTE); + visitor = new MQEVisitor(moduleManager, mock(Entity.class), duration); + } + + @Test + public void rejectsAttributeMissingFromMetricSchema() { + final IllegalExpressionException ex = assertThrows(IllegalExpressionException.class, () -> + visitor.checkAttributes( + "service_relation_client_cpm", + Collections.singletonList(new AttrCondition("attr0", "VIRTUAL_DATABASE", true)))); + assertTrue(ex.getMessage().contains("attr0")); + assertTrue(ex.getMessage().contains("service_relation_client_cpm")); + } + + @Test + public void rejectsNotEqualsAttributeMissingFromMetricSchema() { + // the same holds for `attr0 != x`, since the check is on the attribute key. + assertThrows(IllegalExpressionException.class, () -> + visitor.checkAttributes( + "service_relation_client_cpm", + Collections.singletonList(new AttrCondition("attr0", "MESH", false)))); + } + + @Test + public void allowsAttributePresentInMetricSchema() { + assertDoesNotThrow(() -> + visitor.checkAttributes( + "endpoint_cpm", + Collections.singletonList(new AttrCondition("attr0", "GENERAL", true)))); + } + + @Test + public void allowsEmptyAttributes() { + assertDoesNotThrow(() -> visitor.checkAttributes("service_relation_client_cpm", Collections.emptyList())); + } + + private static Model modelWithColumns(String name, String... columnNames) { + final Model model = mock(Model.class); + when(model.getName()).thenReturn(name); + final List<ModelColumn> columns = new ArrayList<>(columnNames.length); + for (final String columnName : columnNames) { + final ColumnName wrappedName = mock(ColumnName.class); + when(wrappedName.getName()).thenReturn(columnName); + final ModelColumn column = mock(ModelColumn.class); + when(column.getColumnName()).thenReturn(wrappedName); + when(column.shouldIndex()).thenReturn(true); + columns.add(column); + } + when(model.getColumns()).thenReturn(columns); + return model; + } +}
