Skip to content

Add AQL query recording. #21816

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 31 commits into from
Jun 26, 2025
Merged

Add AQL query recording. #21816

merged 31 commits into from
Jun 26, 2025

Conversation

neunhoef
Copy link
Member

@neunhoef neunhoef commented Jun 20, 2025

This adds AQL query recording on a per server level.
This uses the same mechanisms as the API call recording.
A new API is exposed to deliver the recently executed queries.

This API should only be executable with admin access. There is a
config setting to restrict it to superuser or to switch it off in its
entirety.

Scope & Purpose

(Please describe the changes in this PR for reviewers, motivation, rationale - mandatory)

  • [*] 🍕 New feature

Checklist

  • Tests
    • Regression tests
    • C++ Unit tests
    • integration tests
    • resilience tests
  • [*] 📖 CHANGELOG entry made
  • 📚 documentation written (release notes, API changes, ...)
  • [*] Backports: none

@neunhoef neunhoef added this to the devel milestone Jun 20, 2025
@neunhoef neunhoef self-assigned this Jun 20, 2025
@cla-bot cla-bot bot added the cla-signed label Jun 20, 2025
@neunhoef neunhoef marked this pull request as draft June 20, 2025 15:06
@neunhoef neunhoef marked this pull request as ready for review June 25, 2025 14:19
@neunhoef neunhoef requested a review from a team as a code owner June 25, 2025 14:19
// API should exist, but might be disabled or require different permissions
// We accept both success (200) and forbidden (403) as valid responses
// indicating the endpoint exists
assertTrue(doc.code === 200 || doc.code === 403 || doc.code === 404,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we also accept 404, shouldn't that be an error since api was not found?

Suggested change
assertTrue(doc.code === 200 || doc.code === 403 || doc.code === 404,
assertTrue(doc.code === 200 || doc.code === 403 || doc.code === 404,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I now only accept 200. This is just some LLM halluzination, which I did not catch.

@@ -328,3 +353,58 @@ void RestAdminServerHandler::handleApiCalls() {
}
generateOk(rest::ResponseCode::OK, builder.slice());
}

void RestAdminServerHandler::handleAqlQueries() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect this to be named something like handleAqlRecordingQueries

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed.

@neunhoef neunhoef merged commit 6d9e8bf into devel Jun 26, 2025
7 checks passed
@neunhoef neunhoef deleted the feature/aql-query-visibility branch June 26, 2025 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants