Re-Add debug logging for ds listing #8251
Conversation
…eminds/webknossos into add-debug-logging-for-ds-listing
…eminds/webknossos into add-debug-logging-for-ds-listing
…-debug-logging-for-ds-listing
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing error handling and logging in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (6)
app/models/dataset/DatasetService.scala (2)
352-352: LGTM! Enhanced error messages improve debugging capabilities.The addition of dataset IDs to error messages significantly improves traceability and debugging capabilities. This aligns well with the PR's objective of enhancing debug logging for dataset listing operations.
Consider extracting the dataset ID error context into a helper method to maintain DRY principles:
+ private def withDatasetContext(message: String, datasetId: ObjectId): String = + s"$message for dataset $datasetId" - ?~> s"fetching data store failed for dataset ${dataset._id}" + ?~> withDatasetContext("fetching data store failed", dataset._id)Also applies to: 354-362
352-364: Consider adding structured logging for better observability.While the current changes improve error messages, structured logging would provide even better debugging capabilities.
Consider adding structured logging using the LazyLogging trait that's already mixed in:
// Add at the beginning of publicWrites logger.debug( s"Fetching public writes for dataset", Map( "datasetId" -> dataset._id.toString, "organization" -> dataset._organization.toString, "requestingUser" -> requestingUserOpt.map(_._id.toString).getOrElse("none") ) )app/controllers/DatasetController.scala (3)
203-203: Remove unnecessary Fox.successful callThis line doesn't serve any purpose and can be safely removed.
- _ <- Fox.successful(())
204-205: Improve logging structure for better parsingConsider using structured logging with key-value pairs for better log parsing and analysis.
- _ = logger.info( - s"Requesting listing datasets with isActive '$isActive', isUnreported '$isUnreported', organizationId '$organizationIdOpt', folderId '$folderIdValidated', uploaderId '$uploaderIdValidated', searchQuery '$searchQuery', recursive '$recursive', limit '$limit'") + _ = logger.info("Requesting listing datasets", + "isActive" -> isActive, + "isUnreported" -> isUnreported, + "organizationId" -> organizationIdOpt, + "folderId" -> folderIdValidated, + "uploaderId" -> uploaderIdValidated, + "searchQuery" -> searchQuery, + "recursive" -> recursive, + "limit" -> limit) - _ = logger.info(s"Found ${datasets.size} datasets successfully") + _ = logger.info("Dataset listing completed", "count" -> datasets.size)Also applies to: 214-214
227-227: Remove unnecessary Fox.successful callsThese lines don't serve any purpose and can be safely removed.
- _ <- Fox.successful(()) - _ <- Fox.successful(())Also applies to: 238-238
app/models/dataset/Dataset.scala (1)
223-227: Consider enhancing the second log message.While the first log message provides valuable debugging information about selection predicates, the second log message "Requesting datasets with query" could be more informative.
Consider adding more context to the second log message, for example:
- _ = 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 (3)
app/controllers/DatasetController.scala(1 hunks)app/models/dataset/Dataset.scala(2 hunks)app/models/dataset/DatasetService.scala(1 hunks)
🔇 Additional comments (2)
app/models/dataset/DatasetService.scala (1)
364-364: LGTM! Consistent error handling for storage operations.
The error message enhancement for storage operations maintains consistency with the other improvements.
app/models/dataset/Dataset.scala (1)
118-125: LGTM! Enhanced error messages improve debugging capabilities.
The added error messages for parsing operations are clear and specific, making it easier to identify and diagnose parsing issues.
| _ <- Fox.successful(()) | ||
| _ = logger.info(s"datasets: $datasets, requestingUser: ${requestingUser.map(_._id)}") |
There was a problem hiding this comment.
Avoid logging sensitive information
The logging statements include sensitive information like user IDs and dataset details. Consider:
- Masking or truncating sensitive IDs
- Reducing the verbosity of dataset information
- Moving detailed logging to DEBUG level
- _ = logger.info(s"datasets: $datasets, requestingUser: ${requestingUser.map(_._id)}")
+ _ = logger.debug("Processing datasets",
+ "datasetCount" -> datasets.size,
+ "hasRequestingUser" -> requestingUser.isDefined)
- _ = logger.info(s"byOrgaTuple orga: ${byOrgaTuple._1}, datasets: ${byOrgaTuple._2}")
+ _ = logger.debug("Processing organization datasets",
+ "organizationId" -> byOrgaTuple._1,
+ "datasetCount" -> byOrgaTuple._2.size)
- logger.info(s"datastore: ${byDataStoreTuple._1}, datasets: ${byDataStoreTuple._2}")
+ logger.debug("Processing datastore datasets",
+ "datastoreName" -> byDataStoreTuple._1,
+ "datasetCount" -> byDataStoreTuple._2.size)Also applies to: 238-239, 244-244
…hub.com:scalableminds/webknossos into add-debug-logging-for-ds-listing
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/controllers/DatasetController.scala(1 hunks)
🔇 Additional comments (1)
app/controllers/DatasetController.scala (1)
228-228: 🛠️ Refactor suggestion
Reduce logging verbosity in listGrouped method
The previous comment about logging sensitive information applies here as well.
Additionally, consider consolidating these debug statements into a single, structured log entry at the end of the grouping process.
- _ = logger.info(s"datasets: $datasets, requestingUser: ${requestingUser.map(_._id)}")
- _ = logger.info(
- s"requestingUserTeamManagerMemberships: ${requestingUserTeamManagerMemberships.map(_.map(_.toString))}")
- _ = logger.info(s"byOrgaTuple orga: ${byOrgaTuple._1}, datasets: ${byOrgaTuple._2}")
- _ = logger.info(s"datastore: ${byDataStoreTuple._1}, datasets: ${byDataStoreTuple._2}")
+ _ = logger.debug("Dataset grouping completed",
+ "totalDatasets" -> datasets.size,
+ "hasRequestingUser" -> requestingUser.isDefined,
+ "organizationCount" -> groupedByOrga.size,
+ "datastoreCount" -> groupedByDataStore.size)Also applies to: 233-234, 239-239, 244-244
| _ = logger.info( | ||
| s"Requesting listing datasets with isActive '$isActive', isUnreported '$isUnreported', organizationId '$organizationIdOpt', folderId '$folderIdValidated', uploaderId '$uploaderIdValidated', searchQuery '$searchQuery', recursive '$recursive', limit '$limit'") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adjusting log levels and format for production use
The logging statements provide valuable debugging information but consider:
- Moving detailed parameter logging to DEBUG level
- Using structured logging format for better parsing
- Avoiding logging sensitive IDs in production
- _ = logger.info(
- s"Requesting listing datasets with isActive '$isActive', isUnreported '$isUnreported', organizationId '$organizationIdOpt', folderId '$folderIdValidated', uploaderId '$uploaderIdValidated', searchQuery '$searchQuery', recursive '$recursive', limit '$limit'")
+ _ = logger.debug("Requesting dataset listing",
+ "isActive" -> isActive,
+ "isUnreported" -> isUnreported,
+ "hasOrganizationId" -> organizationIdOpt.isDefined,
+ "hasFolderId" -> folderIdValidated.isDefined,
+ "hasUploaderId" -> uploaderIdValidated.isDefined,
+ "hasSearchQuery" -> searchQuery.isDefined,
+ "recursive" -> recursive,
+ "limit" -> limit)
- _ = logger.info(s"Found ${datasets.size} datasets successfully")
+ _ = logger.debug("Dataset listing completed", "count" -> datasets.size)Also applies to: 214-214
This reverts commit 0a2afa4.
* Revert "Re-Add debug logging for ds listing (#8251)" This reverts commit 0a2afa4. * add some error message * Update app/models/dataset/DataStore.scala Co-authored-by: Florian M <[email protected]> --------- Co-authored-by: Michael Büßemeyer <[email protected]> Co-authored-by: Florian M <[email protected]>
2 participants
Seems like #8249 did not fully fix the issue. I still cannot reproduce the error locally (even when working a very similar scenario to what caused the first error).
URL of deployed dev instance (used for testing):
Steps to test: