You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "wu-sheng (via GitHub)" <gi...@apache.org> on 2026/07/07 11:49:44 UTC
[PR] Fix MQE top_n raising a raw storage IO exception for an invalid attribute condition [skywalking]
wu-sheng opened a new pull request, #13940:
URL: https://github.com/apache/skywalking/pull/13940
### Fix `top_n(metric, N, order, attrX='value')` returning a raw storage `IOException` when the attribute is not a column of the metric
- [x] Add a unit test to verify that the fix works.
- [x] Explain briefly why the bug exists and how to fix it.
**Why the bug exists.** An MQE `top_n(...)` query accepts attribute conditions (e.g. `attr0='VIRTUAL_DATABASE'`, `attr0!='MESH'`). These `attr0..attrN` columns exist only on *decorated* metrics — `service_*` / `endpoint_*` / `kubernetes_service_*`, where OAL `.decorator("ServiceDecorator"/"EndpointDecorator"/"K8SServiceDecorator")` sets `attr0` to the entity's layer — plus the MAL `Meter` base class. Metrics whose source declares no attribute columns (service/instance/endpoint **relations**, and **`database_access_*` / `cache_*` / `mq_*`** virtual-access metrics) carry no `attr0` tag at all.
When such an attribute condition is passed for one of those metrics, nothing validated it before the storage call: `MQEVisitor.querySortMetrics` → `AggregationQueryService.sortMetrics` → the storage DAO builds a query filtering on a tag the measure does not define, and the backend fails. On BanyanDB the `BanyanDBException` is wrapped as `IOException`, which `MQEVisitor` flattens into the opaque, non-actionable `"Internal IO exception, query metrics error."` — indistinguishable from a genuine I/O fault. A well-formed MQE should never surface a storage I/O exception.
**How it's fixed.** `MQEVisitor.checkAttributes(...)` now runs before the storage call and validates each attribute key against the metric's registered queryable columns (via `IModelManager`). If the attribute is not a column of the metric it raises `IllegalExpressionException` naming the attribute and the metric, which the existing `catch` in `visitMetric` turns into a descriptive `ExpressionResult` error (the same graceful path already used for "metric does not exist", "TopN value must be > 0", etc.). Decorated metrics and empty attribute lists are unaffected; the check is on the attribute key, so `=` and `!=` conditions are both covered.
The guard lives only in `MQEVisitor` because MQE is the sole caller that populates `TopNCondition.attributes` (PromQL's `buildTopNCondition` never sets them; the GraphQL `TopNCondition` input does not expose `attributes`).
Unit test: `MQEVisitorAttributeTest` (4 cases — reject `=`/`!=` on a no-attr metric, allow a valid attribute, allow empty attributes).
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_017caW5gdjPCYqWprgA6csjJ
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MQE top_n raising a raw storage IO exception for an invalid attribute condition [skywalking]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13940:
URL: https://github.com/apache/skywalking/pull/13940#discussion_r3536190395
##########
oap-server/server-query-plugin/query-graphql-plugin/src/main/java/org/apache/skywalking/oap/query/graphql/mqe/rt/MQEVisitor.java:
##########
@@ -220,11 +235,49 @@ public ExpressionResult visitMetric(MQEParser.MetricContext ctx) {
}
}
+ /**
+ * 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.");
Review Comment:
The exception message uses "can not"; standard English is "cannot". Fixing this improves the user-facing MQE error text.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix MQE top_n raising a raw storage IO exception for an invalid attribute condition [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #13940:
URL: https://github.com/apache/skywalking/pull/13940
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org