Add further debug logging for ds listing #8247
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
app/controllers/DatasetController.scala (1)
179-262: Consider implementing structured loggingTo improve the debugging capabilities while maintaining security and performance, consider implementing structured logging:
- Define a logging schema for dataset operations
- Use MDC (Mapped Diagnostic Context) to track request context
- Implement log sanitization to prevent PII exposure
- Add request tracing for better correlation of logs
Example implementation:
object DatasetLogging { case class DatasetOperationContext( datasetCount: Int, organizationId: String, requestId: String, userId: Option[String] ) def withContext[T](context: DatasetOperationContext)(block: => T): T = { MDC.put("request.id", context.requestId) MDC.put("dataset.count", context.datasetCount.toString) MDC.put("organization.id", context.organizationId) context.userId.foreach(id => MDC.put("user.id", id)) try { block } finally { MDC.clear() } } }app/models/dataset/Dataset.scala (1)
223-227: Consider enhancing the second log message.While the first log message provides valuable information about selection predicates, the second log message "Requesting datasets with query" could be more informative.
Consider this improvement:
- _ = logger.info("Requesting datasets with query") + _ = logger.info(s"Executing dataset query with limit ${limitOpt.getOrElse("none")}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/controllers/DatasetController.scala(1 hunks)app/models/dataset/Dataset.scala(2 hunks)
🔇 Additional comments (1)
app/models/dataset/Dataset.scala (1)
118-125: LGTM! Improved error messages for better debugging.
The error messages are now more specific and descriptive, which will help in identifying parsing issues more effectively.
| m: MessagesProvider): Fox[List[JsObject]] = | ||
| for { | ||
| _ <- Fox.successful(()) | ||
| _ = logger.info(s"datasets: $datasets, requestingUser: $requestingUser") |
There was a problem hiding this comment.
Security & Performance: Revise logging strategy
The current logging implementation has several concerns:
- Security: Logging entire objects (
datasets,requestingUser,requestingUserTeamManagerMemberships) may expose sensitive information - Performance: Excessive logging at multiple levels
- Log levels: Using INFO level for debug information
- _ = logger.info(s"datasets: $datasets, requestingUser: $requestingUser")
+ _ = logger.debug(s"Processing ${datasets.size} datasets for user ${requestingUser.map(_._id).getOrElse("anonymous")}")
- _ = logger.info(s"requestingUserTeamManagerMemberships: $requestingUserTeamManagerMemberships")
+ _ = logger.debug(s"User ${requestingUser.map(_._id).getOrElse("anonymous")} has ${requestingUserTeamManagerMemberships.size} team manager memberships")
- _ = logger.info(s"byOrgaTuple: $byOrgaTuple")
+ _ = logger.debug(s"Processing organization: ${byOrgaTuple._1} with ${byOrgaTuple._2.size} datasets")
- logger.info(s"datastore: ${byDataStoreTuple._1}, datasets: ${byDataStoreTuple._2}")
+ logger.debug(s"Processing datastore: ${byDataStoreTuple._1} with ${byDataStoreTuple._2.size} datasets")Also applies to: 233-233, 238-238, 243-243
| userService | ||
| .teamManagerMembershipsFor(user._id)) ?~> s"Could not find team manager memberships for user ${requestingUser | ||
| .map(_._id)}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error messages for better debugging
The error messages should provide more context about what failed and why.
userService
.teamManagerMembershipsFor(user._id)) ?~> s"Could not find team manager memberships for user ${requestingUser
- .map(_._id)}"
+ .map(_._id)}. Verify user permissions and team associations."
- } ?~> s"Could not group by datastore for datasets ${byOrgaTuple._2.map(_.name)}"
+ } ?~> s"Failed to process datasets by datastore. Organization: ${organization._id}, Datastores: ${groupedByDataStore.map(_._1).mkString(", ")}"
- } ?~> s"Could not group by organization for datasets ${datasets.map(_.name)}"
+ } ?~> s"Failed to process datasets by organization. Dataset count: ${datasets.size}, Organizations: ${groupedByOrga.map(_._1).mkString(", ")}"Also applies to: 260-260, 262-262
| limit) ?~> "dataset.list.failed" ?~> "Dataset listing failed" | ||
| _ = logger.info(s"Found ${datasets.size} datasets successfully") | ||
| js <- listGrouped(datasets, request.identity) ?~> "dataset.list.failed" ?~> "Grouping datasets failed" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and messages
The error handling in these lines could be enhanced:
- Remove the redundant error message "Dataset listing failed" as it duplicates "dataset.list.failed"
- Enhance the error message for grouping failure to include more context
- limit) ?~> "dataset.list.failed" ?~> "Dataset listing failed"
+ limit) ?~> "dataset.list.failed"
_ = logger.info(s"Found ${datasets.size} datasets successfully")
- js <- listGrouped(datasets, request.identity) ?~> "dataset.list.failed" ?~> "Grouping datasets failed"
+ js <- listGrouped(datasets, request.identity) ?~> "dataset.list.grouping.failed" ?~> s"Failed to group ${datasets.size} datasets"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| limit) ?~> "dataset.list.failed" ?~> "Dataset listing failed" | |
| _ = logger.info(s"Found ${datasets.size} datasets successfully") | |
| js <- listGrouped(datasets, request.identity) ?~> "dataset.list.failed" ?~> "Grouping datasets failed" | |
| limit) ?~> "dataset.list.failed" | |
| _ = logger.info(s"Found ${datasets.size} datasets successfully") | |
| js <- listGrouped(datasets, request.identity) ?~> "dataset.list.grouping.failed" ?~> s"Failed to group ${datasets.size} datasets" |
| m: MessagesProvider): Fox[List[JsObject]] = | ||
| for { | ||
| _ <- Fox.successful(()) | ||
| _ = logger.info(s"datasets: $datasets, requestingUser: $requestingUser") |
There was a problem hiding this comment.
| _ = logger.info(s"datasets: $datasets, requestingUser: $requestingUser") | |
| _ = logger.info(s"datasets: $datasets, requestingUser: ${requestingUser._id}") |
There was a problem hiding this comment.
let’s avoid printing entire user objects,
only user objects?
There was a problem hiding this comment.
I just reduced the logging to only id or such and not whole object. Do you think whole object is ok (besides users)? This might make debugging easier 🤔
There was a problem hiding this comment.
Yes, should be ok. This is only temporary, this isn’t super sensitive data and the logs are of course not public.
…eminds/webknossos into add-debug-logging-for-ds-listing
…eminds/webknossos into add-debug-logging-for-ds-listing
The method
listGroupedneeds to be investigated further.Follow up to #8246
Steps to test: