-
Notifications
You must be signed in to change notification settings - Fork 16
[DAPS-1522] User router logging improvements #1629
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
base: devel
Are you sure you want to change the base?
[DAPS-1522] User router logging improvements #1629
Conversation
Reviewer's GuideThis PR enhances the user router by introducing consistent, structured logging around each endpoint’s execution (start, success, failure) and refactors variable declarations to ensure contextual data is available in error handlers. Sequence diagram for enhanced logging in user router endpointssequenceDiagram
participant Client
participant "User Router"
participant Logger
participant "Database/Libs"
Client->>"User Router": HTTP GET /authn/password (or other endpoint)
"User Router"->>Logger: Log Start (with client, correlation ID, route, etc.)
"User Router"->>"Database/Libs": Perform endpoint logic (e.g., authenticate, query, update)
alt Success
"User Router"->>Logger: Log Success (with client, correlation ID, route, etc.)
"User Router"->>Client: Send response
else Failure
"User Router"->>Logger: Log Failure (with client, correlation ID, route, error, stack)
"User Router"->>Client: Send error response
end
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `core/database/foxx/api/user_router.js:271` </location>
<code_context>
+ req.queryParams.email,
+ "Options:",
+ req.queryParams.options,
+ "uuid:",
+ req.queryParams.uuids,
+ "is_admin:",
+ req.queryParams.is_admin,
</code_context>
<issue_to_address>
Potential typo: 'uuid' vs 'uuids' in logging statements.
Update log statements to use 'uuids:' for consistency with the parameter name and to prevent confusion.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
"uuid:",
req.queryParams.uuids,
=======
"uuids:",
req.queryParams.uuids,
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `core/database/foxx/api/user_router.js:1957` </location>
<code_context>
router
.get("/list/all", function (req, res) {
+ let client = undefined;
var qry = "for i in u sort i.name_last, i.name_first";
var result;
+ console.info(
</code_context>
<issue_to_address>
Logging uses 'client' before it is assigned.
Assign 'client' before logging, or remove it from the log if not needed, to avoid logging 'unknown'.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Good initiative on improving observability 💪
Consider using middleware to wrap all router requests at a higher level ⛰️ - this would eliminate the repetitive logging code and make maintenance much easier
Two ideas for approaches:
Example util funciton
// In index.js - handles 90% of your logging needs
router.use((req, res, next) => {
const start = Date.now();
console.info(JSON.stringify({
event: "request_start",
...extractContext(req)
}));
res.on("finish", () => {
console.info(JSON.stringify({
event: "request_end",
duration: Date.now() - start,
status: res.statusCode
}));
});
next();
});
This could reduce the PR from 2000+ lines to ~50. Let me know if you have any questions
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.
- Change to using null
- Fix the spacing here: blic key|Extra: unknown |Err: unknown |Stack: unkn
- Don't print access tokens or passwords
standardized logging (needs more testing)
req.headers['x-correlation-id'] instead of just having placeholder text
slight changes to some log output to obfuscate sensitive data
ee33cc1
to
d8ff808
Compare
Co-authored-by: Joshua S Brown <[email protected]>
Ticket
#1522
Description
First pass of logging improvements within user router
Tasks
Summary by Sourcery
Instrument user router endpoints with structured logging to capture request lifecycle events and contextual metadata
Enhancements: