[1522] Repo Router Logging Improvements #1823
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
megatnt1122
added
Type: Refactor
Contributor
Reviewer's GuideThis PR instruments the repo_router Foxx service with structured request logging for key repo and allocation endpoints and adds integration tests that exercise repo listing, CRUD, size calculation, and allocation-related routes, ensuring responses are shaped correctly for clients. Sequence diagram for repo view request logging in repo_routersequenceDiagram
actor Client
participant RepoRouter
participant Logger
participant RepoCollection as Repo
participant AdminCollection as Admin
participant OwnerCollection as Owner
Client->>RepoRouter: GET /repo/view?id
RepoRouter->>Logger: logRequestStarted(correlationId, httpVerb, routePath, status, description)
RepoRouter->>RepoCollection: document(id)
RepoCollection-->>RepoRouter: repo
RepoRouter->>AdminCollection: byExample({_to: repo._id})
AdminCollection-->>RepoRouter: admins[]
RepoRouter->>OwnerCollection: byExample({_from: repo._id})
OwnerCollection-->>RepoRouter: owners[]
RepoRouter->>RepoRouter: enrich repo (admins, owners, paths)
RepoRouter-->>Client: [repo]
RepoRouter->>Logger: logRequestSuccess(correlationId, httpVerb, routePath, status, description, extra=repo)
rect rgb(255,230,230)
Client->>RepoRouter: GET /repo/view?id (error path)
RepoRouter->>Logger: logRequestStarted(...)
RepoRouter->>RepoCollection: document(id)
RepoCollection-->>RepoRouter: error e
RepoRouter->>Logger: logRequestFailure(correlationId, httpVerb, routePath, status, description, extra=repo, error=e)
RepoRouter->>RepoRouter: handleException(e, res)
RepoRouter-->>Client: error response
end
Sequence diagram for allocation create request logging and task initializationsequenceDiagram
actor Client
participant RepoRouter
participant Logger
participant ArangoDB as g_db
participant Tasks as g_tasks
Client->>RepoRouter: GET /repo/alloc/create?client&repo&subject
RepoRouter->>Logger: logRequestStarted(client, correlationId, httpVerb, routePath, status, description)
RepoRouter->>ArangoDB: _executeTransaction({ read: ["u","uuid","accn","repo","admin"], write: ["task"] })
activate ArangoDB
ArangoDB->>RepoRouter: transaction callback
RepoRouter->>RepoRouter: resolve client and subject_id
RepoRouter->>Tasks: taskInitAllocCreate(client, repo, subject_id, data_limit, rec_limit)
Tasks-->>RepoRouter: result (task, state)
RepoRouter-->>ArangoDB: return result
deactivate ArangoDB
RepoRouter-->>Client: result
RepoRouter->>Logger: logRequestSuccess(client, correlationId, httpVerb, routePath, status, description, extra={task_id, repo, subject, data_limit, rec_limit, status})
rect rgb(255,230,230)
Client->>RepoRouter: GET /repo/alloc/create (error path)
RepoRouter->>Logger: logRequestStarted(...)
RepoRouter->>ArangoDB: _executeTransaction(...)
ArangoDB-->>RepoRouter: error e
RepoRouter->>Logger: logRequestFailure(client, correlationId, httpVerb, routePath, status, description, extra={task_id, repo, subject, data_limit, rec_limit, status}, error=e)
RepoRouter->>RepoRouter: handleException(e, res)
RepoRouter-->>Client: error response
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- Several log entries hardcode
httpVerb: "GET"for non-GET endpoints (e.g./repo/create,/repo/update), which will make log analysis confusing; consider passing the actual method (e.g.req.method). - In
/alloc/create(and similar handlers), the failure logging block referencesresult.task.*even thoughresultcan still benullwhen an error occurs, which risks masking the original error; guard access toresultor log only when it is defined. - In
/alloc/stats, the success path callslogger.logRequestStartedinstead oflogRequestSuccess, which breaks the start/success/failure pattern and may confuse downstream log consumers; use the success variant there.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several log entries hardcode `httpVerb: "GET"` for non-GET endpoints (e.g. `/repo/create`, `/repo/update`), which will make log analysis confusing; consider passing the actual method (e.g. `req.method`).
- In `/alloc/create` (and similar handlers), the failure logging block references `result.task.*` even though `result` can still be `null` when an error occurs, which risks masking the original error; guard access to `result` or log only when it is defined.
- In `/alloc/stats`, the success path calls `logger.logRequestStarted` instead of `logRequestSuccess`, which breaks the start/success/failure pattern and may confuse downstream log consumers; use the success variant there.
## Individual Comments
### Comment 1
<location> `core/database/foxx/api/repo_router.js:108-117` </location>
<code_context>
}
res.send(result);
+ logger.logRequestSuccess({
+ client: req.queryParams?.client,
+ correlationId: req.headers["x-correlation-id"],
+ httpVerb: "GET",
+ routePath: basePath + "/list",
+ status: "Started",
+ description: "List repo servers administered by client",
+ extra: { NumOfRepoServers: result.length },
</code_context>
<issue_to_address>
**suggestion:** Use a success status for the success log entry in /list instead of "Started".
The `/list` `logRequestSuccess` call uses `status: "Started"`, which makes it hard to distinguish start vs success in telemetry. Please change this to a success-specific status (e.g. "Success") or omit `status` if the logger already infers it.
```suggestion
res.send(result);
logger.logRequestSuccess({
client: req.queryParams?.client,
correlationId: req.headers["x-correlation-id"],
httpVerb: "GET",
routePath: basePath + "/list",
status: "Success",
description: "List repo servers administered by client",
extra: { NumOfRepoServers: result.length },
});
```
</issue_to_address>
### Comment 2
<location> `core/database/foxx/api/repo_router.js:53-62` </location>
<code_context>
router
.get("/list", function (req, res) {
var client;
+ logger.logRequestStarted({
+ client: req.queryParams?.client,
+ correlationId: req.headers["x-correlation-id"],
</code_context>
<issue_to_address>
**issue:** Success path in /alloc/stats uses logRequestStarted instead of logRequestSuccess.
In the `/alloc/stats` handler, the success branch is still calling `logger.logRequestStarted` with `status: "Success"`. This is likely a copy/paste error and will mix up start vs. success events in our logs. Please switch this to `logRequestSuccess` to match the other routes and keep logging semantics consistent for downstream consumers.
</issue_to_address>
### Comment 3
<location> `core/database/foxx/tests/repo_router.test.js:414-423` </location>
<code_context>
+ it("should calculate per-repo sizes for specified items", () => {
</code_context>
<issue_to_address>
**issue (testing):** The calc_size test passes even if no size is calculated, so it doesn’t prove the endpoint works.
The test only checks that the response is an array and only inspects the first element if any results are present. As a result, it will still pass if `calc_size` returns an empty array, even though you’ve created a known item (`d/song1`) with a specific size. Please update the test to require a non-empty response and assert the expected `repo` and `size` values (e.g. `expect(json).to.have.lengthOf(1)` and `expect(json[0].size).to.equal(1234)`) so it will fail if size calculation regresses.
</issue_to_address>
### Comment 4
<location> `core/database/foxx/tests/repo_router.test.js:783-778` </location>
<code_context>
+ it("should delete an allocation for a user/project when repo admin", async function () {
</code_context>
<issue_to_address>
**issue (testing):** The allocation delete test depends on implicit state and uses a different request style than other tests, which is likely incorrect.
Two main concerns:
1. This test relies on implicit preconditions: it calls `/repo/alloc/create` without explicitly creating `alloc_admin`, `alloc_user`, the repo, or admin edges. That makes it order-dependent on earlier tests or DB state. Please make it self-contained by adding the necessary setup here or using a shared helper/beforeEach to seed the data.
2. The `request.get` usage differs from other tests: this one passes `{ client: ..., subject: ... }` and reads `createRes.json`, while others build a full URL with query params and parse `response.body`. Unless the harness explicitly supports this style, the request may not hit the intended endpoint. Align this test with the existing pattern to ensure it behaves as expected.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
108
to
+117
| res.send(result); | ||
| logger.logRequestSuccess({ | ||
| client: req.queryParams?.client, | ||
| correlationId: req.headers["x-correlation-id"], | ||
| httpVerb: "GET", | ||
| routePath: basePath + "/list", | ||
| status: "Started", | ||
| description: "List repo servers administered by client", | ||
| extra: { NumOfRepoServers: result.length }, | ||
| }); |
Contributor
There was a problem hiding this comment.
suggestion: Use a success status for the success log entry in /list instead of "Started".
The /list logRequestSuccess call uses status: "Started", which makes it hard to distinguish start vs success in telemetry. Please change this to a success-specific status (e.g. "Success") or omit status if the logger already infers it.
Suggested change
| res.send(result); | |
| logger.logRequestSuccess({ | |
| client: req.queryParams?.client, | |
| correlationId: req.headers["x-correlation-id"], | |
| httpVerb: "GET", | |
| routePath: basePath + "/list", | |
| status: "Started", | |
| description: "List repo servers administered by client", | |
| extra: { NumOfRepoServers: result.length }, | |
| }); | |
| res.send(result); | |
| logger.logRequestSuccess({ | |
| client: req.queryParams?.client, | |
| correlationId: req.headers["x-correlation-id"], | |
| httpVerb: "GET", | |
| routePath: basePath + "/list", | |
| status: "Success", | |
| description: "List repo servers administered by client", | |
| extra: { NumOfRepoServers: result.length }, | |
| }); |
Comment on lines
+53
to
62
| logger.logRequestStarted({ | ||
| client: req.queryParams?.client, | ||
| correlationId: req.headers["x-correlation-id"], | ||
| httpVerb: "GET", | ||
| routePath: basePath + "/list", | ||
| status: "Started", | ||
| description: "List repo servers administered by client", | ||
| }); | ||
| if (req.queryParams.client) { | ||
| client = g_lib.getUserFromClientID(req.queryParams.client); |
Contributor
There was a problem hiding this comment.
issue: Success path in /alloc/stats uses logRequestStarted instead of logRequestSuccess.
In the /alloc/stats handler, the success branch is still calling logger.logRequestStarted with status: "Success". This is likely a copy/paste error and will mix up start vs. success events in our logs. Please switch this to logRequestSuccess to match the other routes and keep logging semantics consistent for downstream consumers.
Comment on lines
+414
to
+423
| it("should calculate per-repo sizes for specified items", () => { | ||
| // arrange | ||
| g_db.u.save(user_params_raw_admin); | ||
|
|
||
| g_db.repo.save({ | ||
| _key: "heavymetal", | ||
| title: "Rock On!!!!", | ||
| capacity: 0, | ||
| type: "metadata", | ||
| }); |
Contributor
There was a problem hiding this comment.
issue (testing): The calc_size test passes even if no size is calculated, so it doesn’t prove the endpoint works.
The test only checks that the response is an array and only inspects the first element if any results are present. As a result, it will still pass if calc_size returns an empty array, even though you’ve created a known item (d/song1) with a specific size. Please update the test to require a non-empty response and assert the expected repo and size values (e.g. expect(json).to.have.lengthOf(1) and expect(json[0].size).to.equal(1234)) so it will fail if size calculation regresses.
|
|
||
| // Optional: check state fields | ||
| expect(task.state).to.have.property("repo_id", "repo/music_repo"); | ||
| expect(task.state).to.have.property("subject", "u/alloc_user"); |
Contributor
There was a problem hiding this comment.
issue (testing): The allocation delete test depends on implicit state and uses a different request style than other tests, which is likely incorrect.
Two main concerns:
- This test relies on implicit preconditions: it calls
/repo/alloc/createwithout explicitly creatingalloc_admin,alloc_user, the repo, or admin edges. That makes it order-dependent on earlier tests or DB state. Please make it self-contained by adding the necessary setup here or using a shared helper/beforeEach to seed the data. - The
request.getusage differs from other tests: this one passes{ client: ..., subject: ... }and readscreateRes.json, while others build a full URL with query params and parseresponse.body. Unless the harness explicitly supports this style, the request may not hit the intended endpoint. Align this test with the existing pattern to ensure it behaves as expected.
JoshuaSBrown
reviewed
Jan 6, 2026
| correlationId: req.headers["x-correlation-id"], | ||
| httpVerb: "GET", | ||
| routePath: basePath + "/list", | ||
| status: "Started", |
Collaborator
There was a problem hiding this comment.
Suggested change
| status: "Started", | |
| status: "Success", |
JoshuaSBrown
reviewed
Jan 6, 2026
Comment on lines
963
to
1000
| res.send(result); | ||
| logger.logRequestSuccess({ | ||
| client: req.queryParams.client, | ||
| correlationId: req.headers["x-correlation-id"], | ||
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/create", | ||
| status: "Success", | ||
| description: `Create user/projects repo allocation: ${req.queryParams.repo}`, | ||
| extra: { | ||
| task_id: result.task._id, | ||
| repo: result.task.state.repo_id, | ||
| subject: result.task.state.subject, | ||
| data_limit: result.task.state.data_limit, | ||
| rec_limit: result.task.state.rec_limit, | ||
| status: "queued", | ||
| }, | ||
| }); | ||
| }, | ||
| }); | ||
| } catch (e) { | ||
| logger.logRequestFailure({ | ||
| client: req.queryParams.client, | ||
| correlationId: req.headers["x-correlation-id"], | ||
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/create", | ||
| status: "Failure", | ||
| description: `Create user/projects repo allocation: ${req.queryParams.repo}`, | ||
| extra: { | ||
| task_id: result.task._id, | ||
| repo: result.task.state.repo_id, | ||
| subject: result.task.state.subject, | ||
| data_limit: result.task.state.data_limit, | ||
| rec_limit: result.task.state.rec_limit, | ||
| status: "queued", | ||
| }, | ||
| error: e, | ||
| }); | ||
| g_lib.handleException(e, res); |
Collaborator
There was a problem hiding this comment.
It would be nice to include the subject in the description as it is a required parameter.
JoshuaSBrown
reviewed
Jan 6, 2026
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/delete", | ||
| status: "Started", | ||
| description: `Delete user/projects repo allocation: ${req.queryParams.repo}`, |
Collaborator
There was a problem hiding this comment.
include the subject in the description.
JoshuaSBrown
reviewed
Jan 6, 2026
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/set", | ||
| status: "Started", | ||
| description: `Set user/projects repo allocation: ${req.queryParams.repo}`, |
Collaborator
There was a problem hiding this comment.
Include the subject in the description
JoshuaSBrown
requested changes
Jan 6, 2026
Collaborator
JoshuaSBrown
left a comment
JoshuaSBrown
left a comment
There was a problem hiding this comment.
Minor suggestions, this is a marked improvement in your work.
JoshuaSBrown
reviewed
Jan 6, 2026
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/create", | ||
| status: "Started", | ||
| description: `Create user/projects repo allocation: ${req.queryParams.repo}`, |
Collaborator
There was a problem hiding this comment.
Include the subject in the descriptions.
JoshuaSBrown
reviewed
Jan 6, 2026
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/create", | ||
| status: "Success", | ||
| description: `Create user/projects repo allocation: ${req.queryParams.repo}`, |
Collaborator
There was a problem hiding this comment.
include subject in description.
JoshuaSBrown
reviewed
Jan 6, 2026
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/create", | ||
| status: "Failure", | ||
| description: `Create user/projects repo allocation: ${req.queryParams.repo}`, |
Collaborator
There was a problem hiding this comment.
Include subject in description.
JoshuaSBrown
reviewed
Jan 6, 2026
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/delete", | ||
| status: "Success", | ||
| description: `Delete user/projects repo allocation: ${req.queryParams.repo}`, |
Collaborator
There was a problem hiding this comment.
include subject in desc
JoshuaSBrown
reviewed
Jan 6, 2026
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/delete", | ||
| status: "Failure", | ||
| description: `Delete user/projects repo allocation: ${req.queryParams.repo}`, |
Collaborator
There was a problem hiding this comment.
include subject in desc
JoshuaSBrown
reviewed
Jan 6, 2026
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/set", | ||
| status: "Success", | ||
| description: `Set user/projects repo allocation: ${req.queryParams.repo}`, |
Collaborator
There was a problem hiding this comment.
include subject in desc
JoshuaSBrown
reviewed
Jan 6, 2026
| httpVerb: "GET", | ||
| routePath: basePath + "/alloc/set", | ||
| status: "Failure", | ||
| description: `Set user/projects repo allocation: ${req.queryParams.repo}`, |
Collaborator
There was a problem hiding this comment.
include subject in desc
JoshuaSBrown
reviewed
Jan 6, 2026
| routePath: basePath + "/view", | ||
| status: "Success", | ||
| description: `View repo server record: ${req.queryParams.id}`, | ||
| extra: repo, |
Collaborator
There was a problem hiding this comment.
We cannot print the full repo doc in the logs this contains secrets.
id: joi.string().required(),
title: joi.string().required(),
desc: joi.string().optional(),
domain: joi.string().optional(),
capacity: joi.number().integer().min(0).required(),
pub_key: joi.string().optional(),
address: joi.string().optional(),
endpoint: joi.string().optional(),
path: joi.string().optional(),
exp_path: joi.string().optional(),
admins: joi.array().items(joi.string()).required(),
type: joi.string().valid(RepositoryType.GLOBUS, RepositoryType.METADATA).optional(),
What you can print is the type, and the capacity
Collaborator
There was a problem hiding this comment.
It would be ideal to also print out the admins, but I'm not sure if that needs to be part of the same log line. Let's discuss.
JoshuaSBrown
reviewed
Jan 6, 2026
| routePath: basePath + "/view", | ||
| status: "Failure", | ||
| description: `View repo server record: ${req.queryParams.id}`, | ||
| extra: repo, |
JoshuaSBrown
reviewed
Jan 6, 2026
| routePath: basePath + "/create", | ||
| status: "Success", | ||
| description: `Create a server record: ${req.queryParams.id}`, | ||
| extra: repo_doc, |
Collaborator
There was a problem hiding this comment.
See comment above don't do this.
JoshuaSBrown
reviewed
Jan 6, 2026
| routePath: basePath + "/create", | ||
| status: "Failure", | ||
| description: `Create a server record: ${req.queryParams.id}`, | ||
| extra: repo_doc, |
Collaborator
There was a problem hiding this comment.
See comment above don't do this.
JoshuaSBrown
reviewed
Jan 6, 2026
| routePath: basePath + "/update", | ||
| status: "Success", | ||
| description: `Update a repo server record: ${req.queryParams.id}`, | ||
| extra: repo?.new, |
Collaborator
There was a problem hiding this comment.
See comment above don't do this.
JoshuaSBrown
reviewed
Jan 6, 2026
| routePath: basePath + "/update", | ||
| status: "Failure", | ||
| description: `Update a repo server record: ${req.queryParams.id}`, | ||
| extra: repo?.new, |
Collaborator
There was a problem hiding this comment.
See comment above don't do this.
JoshuaSBrown
reviewed
Jan 6, 2026
Comment on lines
+759
to
+774
|
|
||
| const requestString = | ||
| `${repo_base_url}/alloc/create?client=${clientId}` + | ||
| `&subject=${subjectId}&repo=${repoId}&data_limit=${dataLimit}&rec_limit=${recLimit}`; | ||
|
|
||
| // Act | ||
| const response = request.get(requestString); | ||
|
|
||
| const json = JSON.parse(response.body); | ||
| expect(json).to.have.property("task"); | ||
|
|
||
| const task = json.task; | ||
| expect(task).to.have.property("_key"); | ||
| expect(task).to.have.property("_id"); | ||
| expect(task).to.have.property("type", 6); | ||
| expect(task).to.have.property("status", 0); |
Collaborator
There was a problem hiding this comment.
The CI is failing around here:
1) should create an allocation for a user/project when repo admin
AssertionError: expected { Object (_key, _id, ...) } to have a property 'status' of 0, but got 1
at Test.fn (/tmp/arangodb-app.root/_db/sdms/api/1/APP/tests/repo_router.test.js:774:30)
at Test.run (/opt/arangodb3/arangodb3-linux-3.12.2_x86_64/usr/share/arangodb3/js/common/modules/@arangodb/mocha/runnable.js:223:14)
at Runner.runTest (/opt/arangodb3/arangodb3-linux-3.12.2_x86_64/usr/share/arangodb3/js/common/modules/@arangodb/mocha/runner.js:298:12)
at /opt/arangodb3/arangodb3-linux-3.12.2_x86_64/usr/share/arangodb3/js/common/modules/@arangodb/mocha/runner.js:385:14
at next (/opt/arangodb3/arangodb3-linux-3.12.2_x86_64/usr/share/arangodb3/js/common/modules/@arangodb/mocha/runner.js:240:16)
at /opt/arangodb3/arangodb3-linux-3.12.2_x86_64/usr/share/arangodb3/js/common/modules/@arangodb/mocha/runner.js:250:9
at next (/opt/arangodb3/arangodb3-linux-3.12.2_x86_64/usr/share/arangodb3/js/common/modules/@arangodb/mocha/runner.js:172:16)
at /opt/arangodb3/arangodb3-linux-3.12.2_x86_64/usr/share/arangodb3/js/common/modules/@arangodb/mocha/runner.js:224:9
at done (/opt/arangodb3/arangodb3-linux-3.12.2_x86_64/usr/share/arangodb3/js/common/modules/@arangodb/mocha/runnable.js:190:7)
at Hook.run (/opt/arangodb3/arangodb3-linux-3.12.2_x86_64/usr/share/arangodb3/js/common/modules/@arangodb/mocha/runnable.js:225:7)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
#1522
Description
Logging Improvements to repo router
Tasks
Summary by Sourcery
Add structured logging to repo router endpoints and expand test coverage for repo listing, viewing, updating, deletion, size calculation, and allocation operations.
Enhancements:
Tests: