-
Notifications
You must be signed in to change notification settings - Fork 16
[1522] Query Router Small Fixes #1820
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
[1522] Query Router Small Fixes #1820
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts the query router request logging for the direct execution endpoint to log only a summary (result count) instead of the full results payload on both success and failure paths. Sequence diagram for updated logging in exec_direct query router endpointsequenceDiagram
actor Client
participant QueryRouter
participant SearchService
participant Logger
Client->>QueryRouter: POST /exec/direct
QueryRouter->>SearchService: executeSearchQuery(payload)
alt Success
SearchService-->>QueryRouter: results
QueryRouter->>Logger: logRequestSuccess(routePath=/exec/direct, extra={count})
Logger-->>QueryRouter: logged
QueryRouter-->>Client: 200 OK with results
else Failure
SearchService-->>QueryRouter: throw error e (optional partial results)
QueryRouter->>Logger: logRequestFailure(routePath=/exec/direct, extra={count}, error=e)
Logger-->>QueryRouter: logged
QueryRouter-->>Client: error response
end
Flow diagram for computing extra count for query router loggingflowchart TD
A[Receive results from search execution] --> B{results is Array?}
B -- Yes --> C[Set count to results.length]
B -- No --> D[Set count to undefined]
C --> E[Build extra object with count]
D --> E[Build extra object with count]
E --> F{Success or Failure?}
F -- Success --> G[Call logRequestSuccess with extra]
F -- Failure --> H[Call logRequestFailure with extra and error]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- In the failure path,
resultsmay be undefined if an exception is thrown before it is assigned; consider initializingresults(e.g., to[]ornull) before thetryso thecountcalculation is predictable in both success and failure cases. - The
extra: { count: ... }logic is duplicated in both the success and failure logging calls; consider extracting a small helper (e.g.,getResultCount(results)) to avoid repeating the same expression and keep the behavior consistent if it changes later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the failure path, `results` may be undefined if an exception is thrown before it is assigned; consider initializing `results` (e.g., to `[]` or `null`) before the `try` so the `count` calculation is predictable in both success and failure cases.
- The `extra: { count: ... }` logic is duplicated in both the success and failure logging calls; consider extracting a small helper (e.g., `getResultCount(results)`) to avoid repeating the same expression and keep the behavior consistent if it changes later.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Please add tests for this: qry/exec/direct so we can make sure it is displaying correctly in the logs. |
…com:ORNL/DataFed into refactor-DAPS-1522-Query-Router-Small-Fixes
Ticket
#1522
Description
Small Fixes to the Query Router
Tasks
Summary by Sourcery
Enhancements: