Skip to content
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

Pass request options to Database.query() etc. #817

Open
Yogu opened this issue Jan 8, 2025 · 2 comments
Open

Pass request options to Database.query() etc. #817

Yogu opened this issue Jan 8, 2025 · 2 comments

Comments

@Yogu
Copy link

Yogu commented Jan 8, 2025

I would like to have a way to pass some request-related options when calling high-level methods like Database.query(), Database.executeTransaction() etc. I'm mainly interested in beforeRequest and afterResponse, but I think all of CommonRequestOptions would make sense. In addition, the options could include a signal to be able to abort a request.

My use cases

  • Our frontends cancel requests when they are no longer needed (e.g. when the user changes filter criteria). If this happens, I would like to abort the request to the ArangoDB server as well, or remove it from the task queue if it's not been started yet. This would be possible by throwing conditionally in beforeRequest, or with a new signal option that I could abort.
  • I would like to track how long a request spent in the arangojs-internal request queue, and correlate this with contextual information. This is not possible with a beforeRequest callback per Database instance, but it would be possible with a beforeRequest option on the .query() call

Other potential use cases

  • Different HTTP timeouts depending on the context (e.g. a longer timeout on queries you know will take a long time)
  • Different values for retryOnConflict depending on the context, e.g. enable them for simple queries, but disable them if you want to retry on a higher level instead, or increase the number of retries for certain queries
  • Set custom HTTP headers like a request ID that will be processed somewhere on the network stack, on a reverse proxy etc.

Suggested API

// New type. Not sure about the name - CommonRequestOptions would be fitting but is already taken for the global options
type ExtraRequestOptions = CommonRequestOptions & {
  signal: AbortSignal

  /**
   * Headers object containing any additional headers to send with the request.
   *
   * Note that the `Authorization` header will be overridden if the `auth`
   * configuration option is set.
   */
  headers?:
    | string[][]
    | Record<string, string | ReadonlyArray<string>>
    | Headers;
}

// Existing type
type QueryOptions = ExtraRequestOptions & {
  // ... existing properties
}

// Existing type
type TransactionOptions = ExtraRequestOptions & {  
  // ... existing properties
}

class Database {
  // Example for a method that currently does not have an options argument
  exists(options: ExtraRequestOptions) {
    // ...
  }
}

The new options could be merged into all option arguments that perform a request. Personally, I'm mostly interested in query, executeTransaction, beginTransaction, Transaction.commit(), Transaction.abort() and the cursor APIs (although we currently don't use cursors).

Alternatively, the request options could also be nested to separate them from the function-specific options

// Existing type
type QueryOptions = {
  // ... existing properties
  requestOptions: ExtraRequestOptions
}
Yogu added a commit to AEB-labs/cruddl that referenced this issue Jan 8, 2025
Removes the "arangojs instrumentation" code, which was a copy of the core
request processing classes of arangojs. The update to arangojs 10 would
have required major changes to these files, and they were a code quality
problem anyway.

Instead of our custom patches, we now rely on beforeRequest and
afterResponse. They can be specified once per Database, which is not
sufficient for our use case, so we currently call request() directly
instead of the proper executeTransaction(). Hopefully, arangojs will add
a way to specify these hooks in executeTransaction() as well. See
arangodb/arangojs#817 for the feature request.

BREAKING CHANGE: arangojs 8 is no longer supported. Update to arangojs 10.

BREAKING CHANGE: ArangoJSConfig is no longer exported. Import ConfigOptions
from arangojs/configuration directly if needed.
Yogu added a commit to AEB-labs/cruddl that referenced this issue Jan 8, 2025
Removes the "arangojs instrumentation" code, which was a copy of the core
request processing classes of arangojs. The update to arangojs 10 would
have required major changes to these files, and they were a code quality
problem anyway.

Instead of our custom patches, we now rely on beforeRequest and
afterResponse. They can be specified once per Database, which is not
sufficient for our use case, so we currently call request() directly
instead of the proper executeTransaction(). Hopefully, arangojs will add
a way to specify these hooks in executeTransaction() as well. See
arangodb/arangojs#817 for the feature request.

BREAKING CHANGE: arangojs 8 is no longer supported. Update to arangojs 10.

BREAKING CHANGE: ArangoJSConfig is no longer exported. Import ConfigOptions
from arangojs/configuration directly if needed.
Yogu added a commit to AEB-labs/cruddl that referenced this issue Jan 8, 2025
Removes the "arangojs instrumentation" code, which was a copy of the core
request processing classes of arangojs. The update to arangojs 10 would
have required major changes to these files, and they were a code quality
problem anyway.

Instead of our custom patches, we now rely on beforeRequest and
afterResponse. They can be specified once per Database, which is not
sufficient for our use case, so we currently call request() directly
instead of the proper executeTransaction(). Hopefully, arangojs will add
a way to specify these hooks in executeTransaction() as well. See
arangodb/arangojs#817 for the feature request.

BREAKING CHANGE: arangojs 8 is no longer supported. Update to arangojs 10.

BREAKING CHANGE: ArangoJSConfig is no longer exported. Import ConfigOptions
from arangojs/configuration directly if needed.
Yogu added a commit to AEB-labs/cruddl that referenced this issue Jan 8, 2025
Removes the "arangojs instrumentation" code, which was a copy of the core
request processing classes of arangojs. The update to arangojs 10 would
have required major changes to these files, and they were a code quality
problem anyway.

Instead of our custom patches, we now rely on beforeRequest and
afterResponse. They can be specified once per Database, which is not
sufficient for our use case, so we currently call request() directly
instead of the proper executeTransaction(). Hopefully, arangojs will add
a way to specify these hooks in executeTransaction() as well. See
arangodb/arangojs#817 for the feature request.

BREAKING CHANGE: arangojs 8 is no longer supported. Update to arangojs 10.

BREAKING CHANGE: ArangoJSConfig is no longer exported. Import ConfigOptions
from arangojs/configuration directly if needed.
@pluma4345
Copy link
Member

Since the beforeRequest and afterResponse callbacks are passed the same request object (via the error or response object in the latter case), you could still identify the request you want to track this way, or am I seeing that wrong? I can see concurrency being an issue tho.

We'll likely not implement an API change like this as we already avoided this for other things that could potentially affect any request like streaming transactions.

@Yogu
Copy link
Author

Yogu commented Jan 13, 2025

Since the beforeRequest and afterResponse callbacks are passed the same request object (via the error or response object in the latter case), you could still identify the request you want to track this way

That would allow to correlated beforeRequest to afterResponse. However, the mentioned use cases do not require correlating these two callbacks to each other, but to the call site.

Going through the "My use cases" (the "other potential use cases" are not related to beforeRequest/afterResponse)

Aborting an ArangoDB request when the user aborted their request

This means I need to throw in beforeRequest (to prevent the ArangoDB request from being sent). afterResponse is not involved at all. To know whether or not I need to throw in beforeRequest, I need context information from the place that called .query() etc., e.g. like this (simplified a lot):

app.get('/', (req, res) => {
  let aborted = false;
  res.once('close', () => { if (!res.writableFinished) { aborted = true; } }); // there should be an easier way but it's broken https://github.com/nodejs/node/issues/46666
  db.query(aql`FOR obj in collection return ...`, {
    beforeRequest: () => { if (aborted) { throw new Error('User aborted the request'); } }
  }).then(result => res.send(...));
});

I don't think there currently is a way to implement this without the feature I proposed here.

Time tracking

While you can implement a very basic time tracking using just beforeRequest and afterResponse, it would not be very useful without any kind of context information. For example, I would like to

  • differentiate between queries and mutations and also other categories
  • for slow queries, generate a log entry with the actual query string (or hash or id or anything that would let me find actual query string that was slow)
  • find out how long a request spent in the arangojs-internal queue (call of .query() to beforeRequest)

All of this would be easy to implement if I could correlate the beforeRequest to the call site, but (as far as I understand) completely impossible without.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants