From 8a37b99279f105166244d4ebf0cc15de88c340c6 Mon Sep 17 00:00:00 2001 From: Jure Bajic Date: Sat, 24 May 2025 22:36:22 +0200 Subject: [PATCH 1/6] Fix query counter gauge --- arangod/Aql/Query.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/arangod/Aql/Query.cpp b/arangod/Aql/Query.cpp index 7303e864c7cd..c52ec8872e36 100644 --- a/arangod/Aql/Query.cpp +++ b/arangod/Aql/Query.cpp @@ -201,9 +201,15 @@ Query::Query(std::shared_ptr ctx, QueryString queryString, : Query(0, ctx, std::move(queryString), std::move(bindParameters), std::move(options), std::make_shared(ctx->vocbase().server(), - scheduler)) {} + scheduler)) { + auto& feature = vocbase().server().getFeature(); + feature.trackQueryStart(); +} Query::~Query() { + auto& feature = vocbase().server().getFeature(); + feature.trackQueryEnd(executionTime()); + if (!_planSliceCopy.isNone()) { _resourceMonitor->decreaseMemoryUsage(_planSliceCopy.byteSize()); } @@ -1952,10 +1958,6 @@ void Query::handlePostProcessing(QueryList& querylist) { void Query::handlePostProcessing() { // elapsed time since query start - auto& queryRegistryFeature = - vocbase().server().getFeature(); - queryRegistryFeature.trackQueryEnd(executionTime()); - if (!queryOptions().skipAudit && ServerState::instance()->isSingleServerOrCoordinator()) { try { @@ -2521,10 +2523,6 @@ void Query::instantiatePlan(velocypack::Slice snippets) { } _queryProfile->registerInQueryList(); - - auto& feature = vocbase().server().getFeature(); - feature.trackQueryStart(); - enterState(QueryExecutionState::ValueType::EXECUTION); } From 106815a03ce8eb3c3f69c10e788c4422dd88ed15 Mon Sep 17 00:00:00 2001 From: Jure Bajic Date: Mon, 26 May 2025 12:24:48 +0200 Subject: [PATCH 2/6] Scope just the execution --- arangod/Aql/Query.cpp | 22 +++++++++++++++------- arangod/Aql/Query.h | 2 ++ 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/arangod/Aql/Query.cpp b/arangod/Aql/Query.cpp index c52ec8872e36..4a720d8a88c3 100644 --- a/arangod/Aql/Query.cpp +++ b/arangod/Aql/Query.cpp @@ -46,6 +46,7 @@ #include "Aql/QueryRegistry.h" #include "Aql/SharedQueryState.h" #include "Aql/Timing.h" +#include "Assertions/Assert.h" #include "Auth/Common.h" #include "Basics/Exceptions.h" #include "Basics/ResourceUsage.h" @@ -201,15 +202,10 @@ Query::Query(std::shared_ptr ctx, QueryString queryString, : Query(0, ctx, std::move(queryString), std::move(bindParameters), std::move(options), std::make_shared(ctx->vocbase().server(), - scheduler)) { - auto& feature = vocbase().server().getFeature(); - feature.trackQueryStart(); -} + scheduler)) {} Query::~Query() { - auto& feature = vocbase().server().getFeature(); - feature.trackQueryEnd(executionTime()); - + TRI_ASSERT(!_isExecuting); if (!_planSliceCopy.isNone()) { _resourceMonitor->decreaseMemoryUsage(_planSliceCopy.byteSize()); } @@ -696,6 +692,11 @@ ExecutionState Query::execute(QueryResult& queryResult) { << elapsedSince(_startTime) << " Query::execute" << " this: " << (uintptr_t)this; + auto& queryRegistryFeature = + vocbase().server().getFeature(); + queryRegistryFeature.trackQueryStart(); + _isExecuting = true; + try { if (killed()) { THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_KILLED); @@ -1859,6 +1860,13 @@ void Query::enterState(QueryExecutionState::ValueType state) { ExecutionState Query::cleanupPlanAndEngine(bool sync) { ensureExecutionTime(); + if (_isExecuting) { + auto& queryRegistryFeature = + vocbase().server().getFeature(); + queryRegistryFeature.trackQueryEnd(executionTime()); + _isExecuting = false; + } + { std::unique_lock guard{_resultMutex}; if (!_postProcessingDone) { diff --git a/arangod/Aql/Query.h b/arangod/Aql/Query.h index 6d85e0018e19..bc11ecfa9593 100644 --- a/arangod/Aql/Query.h +++ b/arangod/Aql/Query.h @@ -506,6 +506,8 @@ class Query : public QueryContext, public std::enable_shared_from_this { // If the query object was constructed from cache // the consequence is that _ast is nullptr bool _isCached{false}; + + bool _isExecuting{false}; }; } // namespace aql From 9f03dbbbfd3a1bcd77b43f87643dacb67ad3e622 Mon Sep 17 00:00:00 2001 From: Jure Bajic Date: Mon, 26 May 2025 13:44:00 +0200 Subject: [PATCH 3/6] Add changelog entry --- CHANGELOG | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG b/CHANGELOG index 1b5a1ce01a9c..a1f24594a0f7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,8 @@ devel ----- +* Fix an integer underflow with gauge arangodb_aql_current_query. + * Fix a possible crash when requesting the async registry: Does now not request the thread name of a potentially destroyed thread. From b069c5a3ee1543f06b6ea0afd9d6f37340b381c5 Mon Sep 17 00:00:00 2001 From: Jure Bajic Date: Tue, 27 May 2025 12:08:04 +0200 Subject: [PATCH 4/6] Add tests --- arangod/Aql/Query.cpp | 35 +++++++--- arangod/Aql/Query.h | 6 ++ tests/js/client/aql/aql-query-metrics.js | 89 ++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 11 deletions(-) create mode 100644 tests/js/client/aql/aql-query-metrics.js diff --git a/arangod/Aql/Query.cpp b/arangod/Aql/Query.cpp index 4a720d8a88c3..0da27e2438ac 100644 --- a/arangod/Aql/Query.cpp +++ b/arangod/Aql/Query.cpp @@ -692,11 +692,6 @@ ExecutionState Query::execute(QueryResult& queryResult) { << elapsedSince(_startTime) << " Query::execute" << " this: " << (uintptr_t)this; - auto& queryRegistryFeature = - vocbase().server().getFeature(); - queryRegistryFeature.trackQueryStart(); - _isExecuting = true; - try { if (killed()) { THROW_ARANGO_EXCEPTION(TRI_ERROR_QUERY_KILLED); @@ -754,6 +749,8 @@ ExecutionState Query::execute(QueryResult& queryResult) { TRI_ASSERT(queryResult.data != nullptr); TRI_ASSERT(queryResult.data->isOpenArray()); TRI_ASSERT(_trx != nullptr); + // We should do this only once + trackExecutionStart(); if (useQueryCache && (isModificationQuery() || !_warnings.empty() || !_ast->root()->isCacheable())) { @@ -926,6 +923,8 @@ QueryResultV8 Query::executeV8(v8::Isolate* isolate) { LOG_TOPIC("6cac7", DEBUG, Logger::QUERIES) << elapsedSince(_startTime) << " Query::executeV8" << " this: " << (uintptr_t)this; + trackExecutionStart(); + ScopeGuard guard([this]() noexcept { trackExecutionEnd(); }); QueryResultV8 queryResult; @@ -1719,6 +1718,25 @@ void Query::logAtEnd() const { } } +void Query::trackExecutionStart() noexcept { + // We should do this only once + if (_isExecuting) { + auto& queryRegistryFeature = + vocbase().server().getFeature(); + queryRegistryFeature.trackQueryStart(); + _isExecuting = true; + } +} + +void Query::trackExecutionEnd() noexcept { + if (_isExecuting) { + auto& queryRegistryFeature = + vocbase().server().getFeature(); + queryRegistryFeature.trackQueryEnd(executionTime()); + _isExecuting = false; + } +} + std::string Query::extractQueryString(size_t maxLength, bool show) const { if (!show) { return ""; @@ -1860,12 +1878,7 @@ void Query::enterState(QueryExecutionState::ValueType state) { ExecutionState Query::cleanupPlanAndEngine(bool sync) { ensureExecutionTime(); - if (_isExecuting) { - auto& queryRegistryFeature = - vocbase().server().getFeature(); - queryRegistryFeature.trackQueryEnd(executionTime()); - _isExecuting = false; - } + trackExecutionEnd(); { std::unique_lock guard{_resultMutex}; diff --git a/arangod/Aql/Query.h b/arangod/Aql/Query.h index bc11ecfa9593..762929ded7e8 100644 --- a/arangod/Aql/Query.h +++ b/arangod/Aql/Query.h @@ -368,6 +368,12 @@ class Query : public QueryContext, public std::enable_shared_from_this { // log the end of a query (warnings only) void logAtEnd() const; + // set the isExecuting flag to true and change execution queries gauge + void trackExecutionStart() noexcept; + + // set the isExecuting flag to false and change execution queries gauge + void trackExecutionEnd() noexcept; + struct CollectionSerializationFlags { bool includeNumericIds = true; bool includeViews = true; diff --git a/tests/js/client/aql/aql-query-metrics.js b/tests/js/client/aql/aql-query-metrics.js new file mode 100644 index 000000000000..487b405057b8 --- /dev/null +++ b/tests/js/client/aql/aql-query-metrics.js @@ -0,0 +1,89 @@ +/*jshint globalstrict:false, strict:false, maxlen: 500 */ +/*global assertEqual, assertTrue, assertFalse, assertNotEqual, assertUndefined, assertNotUndefined, assertMatch, assertNotMatch, fail */ + +// ////////////////////////////////////////////////////////////////////////////// +// / DISCLAIMER +// / +// / Copyright 2014-2025 ArangoDB GmbH, Cologne, Germany +// / Copyright 2004-2014 triAGENS GmbH, Cologne, Germany +// / +// / Licensed under the Business Source License 1.1 (the "License"); +// / you may not use this file except in compliance with the License. +// / You may obtain a copy of the License at +// / +// / https://github.com/arangodb/arangodb/blob/devel/LICENSE +// / +// / 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. +// / +// / Copyright holder is ArangoDB GmbH, Cologne, Germany +// / +/// @author Jure Bajic +// ////////////////////////////////////////////////////////////////////////////// + +const jsunity = require("jsunity"); +const db = require("@arangodb").db; +const internal = require("internal"); +const { + getMetricSingle, + getAllMetric, + getCompleteMetricsValues, +} = require("@arangodb/test-helper"); + +function QueryMetricsTestSuite() { + const dbName = "metricsTestDB"; + const collName = "col"; + let collection; + + return { + setUpAll: function () { + db._createDatabase(dbName); + db._useDatabase(dbName); + + collection = db._create(collName, { + numberOfShards: 3, + }); + + let docs = []; + for (let i = 0; i < 1000; ++i) { + docs.push({ value: i }); + } + collection.save(docs); + }, + + tearDownAll: function () { + db._useDatabase("_system"); + db._dropDatabase(dbName); + }, + + testMetricAqlCurrentQueryNoRunningQueries: function () { + const aqlCurrentQueryMetric = getMetricSingle( + "arangodb_aql_current_query", + ); + assertEqual(aqlCurrentQueryMetric, 0); + }, + + testMetricAqlCurrentQueryWithRunningQueries: function () { + let aqlCurrentQueryMetric = getMetricSingle("arangodb_aql_current_query"); + assertEqual(aqlCurrentQueryMetric, 0); + + for (let i = 0; i < 1000; ++i) { + const query = aql` + FOR d IN ${collection} + FILTER d.value > ${i} + LIMIT 3 + RETURN d`; + db._query(query); + } + + aqlCurrentQueryMetric = getMetricSingle("arangodb_aql_current_query"); + assertEqual(aqlCurrentQueryMetric, 0); + }, + }; +} + +jsunity.run(QueryMetricsTestSuite); +return jsunity.done(); From 58f6f2069ee88516568569eb3502a34715a572a4 Mon Sep 17 00:00:00 2001 From: Jure Bajic Date: Tue, 27 May 2025 14:22:22 +0200 Subject: [PATCH 5/6] Fix condition --- arangod/Aql/Query.cpp | 2 +- tests/js/client/aql/aql-query-metrics.js | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/arangod/Aql/Query.cpp b/arangod/Aql/Query.cpp index 0da27e2438ac..11fefe6e5185 100644 --- a/arangod/Aql/Query.cpp +++ b/arangod/Aql/Query.cpp @@ -1720,7 +1720,7 @@ void Query::logAtEnd() const { void Query::trackExecutionStart() noexcept { // We should do this only once - if (_isExecuting) { + if (!_isExecuting) { auto& queryRegistryFeature = vocbase().server().getFeature(); queryRegistryFeature.trackQueryStart(); diff --git a/tests/js/client/aql/aql-query-metrics.js b/tests/js/client/aql/aql-query-metrics.js index 487b405057b8..d04216655c5d 100644 --- a/tests/js/client/aql/aql-query-metrics.js +++ b/tests/js/client/aql/aql-query-metrics.js @@ -27,11 +27,7 @@ const jsunity = require("jsunity"); const db = require("@arangodb").db; const internal = require("internal"); -const { - getMetricSingle, - getAllMetric, - getCompleteMetricsValues, -} = require("@arangodb/test-helper"); +const getMetricSingle = require("@arangodb/test-helper").getMetricSingle; function QueryMetricsTestSuite() { const dbName = "metricsTestDB"; From a549db396bd5ab07b7d0406d39d30b31bf5014b3 Mon Sep 17 00:00:00 2001 From: Jure Bajic Date: Tue, 27 May 2025 14:31:55 +0200 Subject: [PATCH 6/6] Fix eslint --- tests/js/client/aql/aql-query-metrics.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/js/client/aql/aql-query-metrics.js b/tests/js/client/aql/aql-query-metrics.js index d04216655c5d..0cf272643891 100644 --- a/tests/js/client/aql/aql-query-metrics.js +++ b/tests/js/client/aql/aql-query-metrics.js @@ -28,6 +28,8 @@ const jsunity = require("jsunity"); const db = require("@arangodb").db; const internal = require("internal"); const getMetricSingle = require("@arangodb/test-helper").getMetricSingle; +const arangodb = require("@arangodb"); +const aql = arangodb.aql; function QueryMetricsTestSuite() { const dbName = "metricsTestDB";