[DAPS-1522] Metrics Router Logging Improvements#1797
Merged
megatnt1122 merged 13 commits intodevelfrom Dec 4, 2025
Merged
Conversation
… of github.com:ORNL/DataFed into refactor-DAPS-1522-Metrics-Router-Logging-Improvements
megatnt1122
added
Component: Database
Contributor
Reviewer's GuideAdds structured request logging to the metrics Foxx router, introduces unit tests for its endpoints, wires the metrics router tests into CMake, and applies minor JS formatting cleanups in a third‑party worker file. Sequence diagram for metrics_router msg_count update with structured loggingsequenceDiagram
actor Client
participant MetricsRouter
participant g_lib
participant Logger
participant DB
Client->>MetricsRouter: POST /metrics/msg_count/update?client=clientId
MetricsRouter->>g_lib: getUserFromClientID(clientId)
g_lib-->>MetricsRouter: client
MetricsRouter->>Logger: logRequestStarted(clientId, correlationId, POST, metrics/msg_count/update)
alt success
loop for each timestamp bucket
MetricsRouter->>DB: metrics.save(obj)
DB-->>MetricsRouter: ack
end
MetricsRouter->>Logger: logRequestSuccess(clientId, correlationId, POST, metrics/msg_count/update, obj)
MetricsRouter-->>Client: 200 OK
else failure
MetricsRouter->>Logger: logRequestFailure(clientId, correlationId, POST, metrics/msg_count/update, obj, error)
MetricsRouter->>g_lib: handleException(error, res)
g_lib-->>Client: error response
end
Sequence diagram for metrics_router GET endpoints with structured loggingsequenceDiagram
actor Client
participant MetricsRouter
participant g_lib
participant Logger
participant DB
Client->>MetricsRouter: GET /metrics/msg_count?client=clientId
MetricsRouter->>g_lib: getUserFromClientID(clientId)
g_lib-->>MetricsRouter: client
MetricsRouter->>Logger: logRequestStarted(clientId, correlationId, GET, metrics/msg_count)
alt success
MetricsRouter->>DB: _query(AQL for msg_count)
DB-->>MetricsRouter: result
MetricsRouter->>Logger: logRequestSuccess(clientId, correlationId, GET, metrics/msg_count, result)
MetricsRouter-->>Client: 200 OK with result
else failure
MetricsRouter->>Logger: logRequestFailure(clientId, correlationId, GET, metrics/msg_count, result, error)
MetricsRouter->>g_lib: handleException(error, res)
g_lib-->>Client: error response
end
Client->>MetricsRouter: GET /metrics/users/active?client=clientId
MetricsRouter->>g_lib: getUserFromClientID(clientId)
g_lib-->>MetricsRouter: client or null
MetricsRouter->>Logger: logRequestStarted(clientId, correlationId, GET, metrics/users/active)
alt success
MetricsRouter->>DB: _query(AQL for active users)
DB-->>MetricsRouter: cursor
MetricsRouter->>MetricsRouter: aggregate cursor into cnt
MetricsRouter->>Logger: logRequestSuccess(clientId, correlationId, GET, metrics/users/active, cnt)
MetricsRouter-->>Client: 200 OK with cnt
else failure
MetricsRouter->>Logger: logRequestFailure(clientId, correlationId, GET, metrics/users/active, cnt, error)
MetricsRouter->>g_lib: handleException(error, res)
g_lib-->>Client: error response
end
Client->>MetricsRouter: POST /metrics/purge
MetricsRouter->>Logger: logRequestStarted(undefined, correlationId, POST, metrics/purge)
alt success
MetricsRouter->>DB: metrics.save(purge audit document)
DB-->>MetricsRouter: ack
MetricsRouter->>DB: _query(remove old metrics)
DB-->>MetricsRouter: ack
MetricsRouter->>Logger: logRequestSuccess(undefined, correlationId, POST, metrics/purge, undefined)
MetricsRouter-->>Client: 200 OK
else failure
MetricsRouter->>Logger: logRequestFailure(undefined, correlationId, POST, metrics/purge, undefined, error)
MetricsRouter->>g_lib: handleException(error, res)
g_lib-->>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 there - I've reviewed your changes - here's some feedback:
- Several log entries reuse the description "Update message metrics" for GET /msg_count and other non-update routes; consider using route-specific descriptions (e.g., "Get message metrics") so log messages more accurately reflect the operation being performed.
- In the /purge route logging,
clientandextraare hard-coded to the literal string "undefined"; usingnullor omitting these fields entirely would avoid conflating the string with an actual undefined/missing value in downstream log consumers. - The
POST /purgetest assumesdb.metrics.toArray()[0]is the non-purged document, but collection iteration order is not guaranteed; it would be more robust to locate the remaining document by itstotalortimestamprather than relying on array position.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several log entries reuse the description "Update message metrics" for GET /msg_count and other non-update routes; consider using route-specific descriptions (e.g., "Get message metrics") so log messages more accurately reflect the operation being performed.
- In the /purge route logging, `client` and `extra` are hard-coded to the literal string "undefined"; using `null` or omitting these fields entirely would avoid conflating the string with an actual undefined/missing value in downstream log consumers.
- The `POST /purge` test assumes `db.metrics.toArray()[0]` is the non-purged document, but collection iteration order is not guaranteed; it would be more robust to locate the remaining document by its `total` or `timestamp` rather than relying on array position.
## Individual Comments
### Comment 1
<location> `core/database/foxx/api/metrics_router.js:223-232` </location>
<code_context>
.post("/purge", function (req, res) {
try {
+ logger.logRequestStarted({
+ client: "undefined",
+ correlationId: req.headers["x-correlation-id"],
+ httpVerb: "POST",
</code_context>
<issue_to_address>
**suggestion:** Using the string literal "undefined" in purge route logs may cause confusion versus actual `undefined`/`null` values.
In the `/purge` handler, `client` (and later `extra`) are always logged as the string "undefined", which is indistinguishable from a bug where the value was actually `undefined`. Consider using `null`, omitting the field, or a more explicit value like "no_client" to keep logs unambiguous for analysis and debugging.
Suggested implementation:
```javascript
.post("/purge", function (req, res) {
try {
logger.logRequestStarted({
+ client: null,
+ correlationId: req.headers["x-correlation-id"],
+ httpVerb: "POST",
routePath: basePath + "/msg_count/update",
status: "Started",
description: "Update message metrics",
});
```
1. If there is a corresponding `extra: "undefined"` field later in the same `/purge` handler (or related logging for this route), it should be updated analogously to use `null` or a more explicit sentinel value (e.g. `"no_extra"`) instead of the string `"undefined"`.
2. If a real client identifier is available on `req` (e.g. `req.user`, `req.headers["x-client-id"]`, or similar), consider passing that actual value instead of `null` to further improve log usefulness.
</issue_to_address>
### Comment 2
<location> `core/database/foxx/tests/metrics_router.test.js:212-215` </location>
<code_context>
+ expect(arr[0].uid).to.equal("u2");
+ });
+
+ it("POST /purge should remove metrics older than timestamp", () => {
+ const now = Math.floor(Date.now() / 1000);
+
+ db.metrics.save([
+ { timestamp: now - 1000, type: "msgcnt_total", total: 1 }, // should be removed
+ { timestamp: now, type: "msgcnt_total", total: 2 }, // should stay
+ ]);
+
+ const ts = now - 500;
+ const res = request.post(`${metrics_base_url}/purge?timestamp=${ts}`);
+
+ expect(res.status).to.equal(204);
+
+ const docs = db.metrics.toArray();
+ //Equals 2 due to writing the purge doc
+ expect(docs.length).to.equal(2);
+ expect(docs[0].total).to.equal(2);
+ });
+});
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid relying on implicit document order in the `/purge` test assertions.
In this test, the assertion `expect(docs[0].total).to.equal(2);` assumes `toArray()` returns the surviving metric first, but collection iteration order isn’t guaranteed and may change across backends or ArangoDB versions, making the test flaky.
Instead of indexing into `docs[0]`, assert based on content, e.g.:
```js
const docs = db.metrics.toArray();
expect(docs.length).to.equal(2);
const remaining = docs.find((d) => d.total === 2);
expect(remaining).to.exist;
```
You could also explicitly assert there is exactly one non-`purge` document and one `type === "purge"` document to better reflect the route’s behavior.
```suggestion
const docs = db.metrics.toArray();
// Equals 2 due to writing the purge doc
expect(docs.length).to.equal(2);
// Assert based on content instead of relying on implicit document order
const remainingMetricDocs = docs.filter((d) => d.type !== "purge");
const purgeDocs = docs.filter((d) => d.type === "purge");
expect(remainingMetricDocs.length).to.equal(1);
expect(remainingMetricDocs[0].total).to.equal(2);
expect(purgeDocs.length).to.equal(1);
```
</issue_to_address>
### Comment 3
<location> `web/static/ace/worker-coffee.js:1646-1651` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 4
<location> `web/static/ace/worker-coffee.js:26752-26757` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 5
<location> `web/static/ace/worker-coffee.js:32947-32954` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 6
<location> `web/static/ace/worker-coffee.js:34737-34744` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 7
<location> `web/static/ace/worker-coffee.js:35654-35661` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>
### Comment 8
<location> `web/static/ace/worker-coffee.js:36613-36620` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Use `const` or `let` instead of `var`. ([`avoid-using-var`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/JavaScript/Default-Rules/avoid-using-var))
<details><summary>Explanation</summary>`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code).
`let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than
function-scoped.
From the [Airbnb JavaScript Style Guide](https://airbnb.io/javascript/#references--prefer-const)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
JoshuaSBrown
reviewed
Dec 1, 2025
Collaborator
|
Looks like there might be an error in one of your tests. |
JoshuaSBrown
requested changes
Dec 1, 2025
Collaborator
JoshuaSBrown
left a comment
JoshuaSBrown
left a comment
There was a problem hiding this comment.
There is an error in one of your tests, I'm also seeing that the formatted files are being included again.
JoshuaSBrown
reviewed
Dec 2, 2025
… of github.com:ORNL/DataFed into refactor-DAPS-1522-Metrics-Router-Logging-Improvements
JoshuaSBrown
approved these changes
Dec 3, 2025
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 metric_router
Tasks
Summary by Sourcery
Improve observability and test coverage for the metrics Foxx router.
Enhancements:
Tests: