Debug logging segment to agglomerate on 25 07 #8764
Conversation
📝 WalkthroughWalkthroughThis update finalizes the 25.07.0 WEBKNOSSOS release, aggregating changelog and migration guide entries from individual files. It introduces performance instrumentation and logging for agglomerate ID retrieval across multiple services, updates method signatures for logging control, and adds new features and fixes as documented in the released changelog and migration guide. Changes
Possibly related PRs
Suggested labels
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 0
🧹 Nitpick comments (2)
CHANGELOG.released.md (2)
14-17: Consider dropping duplicate bullets to keep the changelog concise.The two bullets
- Added the ability to duplicate trees in skeleton annotations … - Added a new Account Settings page …appear once under Highlights (L14-17) and again under Added (L22-24).
Including them in Highlights is enough—repeating them verbatim below adds noise without new information.@@ - - Added the ability to duplicate trees in skeleton annotations. Users can create a copy of any tree (including all nodes, edges, and properties) via the context menu in the skeleton tab. [#8662](https://github.com/scalableminds/webknossos/pull/8662) - - Added a new Account Settings page for managing user preferences, passwords and authentication methods. [#8672](https://github.com/scalableminds/webknossos/pull/8672)Also applies to: 22-24
36-38: Placement of “Improved efficiency …” bullet seems off.“Improved efficiency of saving bounding box related annotation changes” is listed under Fixed (L36-38) but it describes a performance optimisation rather than a bug fix.
Moving it to Changed (or keeping only the Highlights entry) would better align with Keep-a-Changelog conventions and avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
CHANGELOG.released.md(1 hunks)MIGRATIONS.released.md(2 hunks)unreleased_changes/8492.md(0 hunks)unreleased_changes/8518.md(0 hunks)unreleased_changes/8542.md(0 hunks)unreleased_changes/8598.md(0 hunks)unreleased_changes/8622.md(0 hunks)unreleased_changes/8633.md(0 hunks)unreleased_changes/8638.md(0 hunks)unreleased_changes/8662.md(0 hunks)unreleased_changes/8672.md(0 hunks)unreleased_changes/8676.md(0 hunks)unreleased_changes/8677.md(0 hunks)unreleased_changes/8679.md(0 hunks)unreleased_changes/8682.md(0 hunks)unreleased_changes/8683.md(0 hunks)unreleased_changes/8686.md(0 hunks)unreleased_changes/8687.md(0 hunks)unreleased_changes/8688.md(0 hunks)unreleased_changes/8695.md(0 hunks)unreleased_changes/8698.md(0 hunks)unreleased_changes/8702.md(0 hunks)unreleased_changes/8703.md(0 hunks)unreleased_changes/8705.md(0 hunks)unreleased_changes/8709.md(0 hunks)unreleased_changes/8710.md(0 hunks)unreleased_changes/8716.md(0 hunks)unreleased_changes/8720.md(0 hunks)unreleased_changes/8721.md(0 hunks)unreleased_changes/8725.md(0 hunks)unreleased_changes/8726.md(0 hunks)unreleased_changes/8727.md(0 hunks)unreleased_changes/8728.md(0 hunks)unreleased_changes/8733.md(0 hunks)util/src/main/scala/com/scalableminds/util/time/Instant.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/Hdf5AgglomerateService.scala(4 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/EditableMappingController.scala(2 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala(3 hunks)
💤 Files with no reviewable changes (32)
- unreleased_changes/8720.md
- unreleased_changes/8622.md
- unreleased_changes/8725.md
- unreleased_changes/8492.md
- unreleased_changes/8726.md
- unreleased_changes/8733.md
- unreleased_changes/8662.md
- unreleased_changes/8598.md
- unreleased_changes/8721.md
- unreleased_changes/8686.md
- unreleased_changes/8716.md
- unreleased_changes/8728.md
- unreleased_changes/8633.md
- unreleased_changes/8703.md
- unreleased_changes/8676.md
- unreleased_changes/8710.md
- unreleased_changes/8695.md
- unreleased_changes/8518.md
- unreleased_changes/8677.md
- unreleased_changes/8679.md
- unreleased_changes/8705.md
- unreleased_changes/8727.md
- unreleased_changes/8542.md
- unreleased_changes/8687.md
- unreleased_changes/8688.md
- unreleased_changes/8638.md
- unreleased_changes/8702.md
- unreleased_changes/8672.md
- unreleased_changes/8709.md
- unreleased_changes/8683.md
- unreleased_changes/8682.md
- unreleased_changes/8698.md
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: CHANGELOG.unreleased.md:14-14
Timestamp: 2024-11-25T14:52:23.201Z
Learning: For the `webknossos` project, changelog entries in `CHANGELOG.unreleased.md` should be concise and avoid unnecessary detailed explanations.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
Learnt from: frcroth
PR: scalableminds/webknossos#8609
File: app/models/dataset/Dataset.scala:753-775
Timestamp: 2025-05-12T13:07:29.637Z
Learning: In the `updateMags` method of DatasetMagsDAO (Scala), the code handles different dataset types distinctly:
1. Non-WKW datasets have `magsOpt` populated and use the first branch which includes axisOrder, channelIndex, and credentialId.
2. WKW datasets will have `wkwResolutionsOpt` populated and use the second branch which includes cubeLength.
3. The final branch is a fallback for legacy data.
This ensures appropriate fields are populated for each dataset type.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/EditableMappingController.scala (2)
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
util/src/main/scala/com/scalableminds/util/time/Instant.scala (3)
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala:161-166
Timestamp: 2025-04-28T14:18:04.368Z
Learning: In Scala for-comprehensions with the Fox error handling monad, `Fox.fromBool()` expressions should use the `<-` binding operator instead of the `=` assignment operator to properly propagate error conditions. Using `=` will cause validation failures to be silently ignored.
MIGRATIONS.released.md (1)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8221
File: CHANGELOG.unreleased.md:14-14
Timestamp: 2024-11-25T14:52:23.201Z
Learning: For the `webknossos` project, changelog entries in `CHANGELOG.unreleased.md` should be concise and avoid unnecessary detailed explanations.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/Hdf5AgglomerateService.scala (1)
Learnt from: frcroth
PR: scalableminds/webknossos#8202
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DatasetErrorLoggingService.scala:68-72
Timestamp: 2024-11-25T10:02:03.702Z
Learning: In `DatasetErrorLoggingService.scala`, prefer using `TextUtils.stackTraceAsString(exception)` when logging exceptions instead of passing the exception directly to `logger.error`.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (3)
Learnt from: frcroth
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/MeshFileService.scala:170-173
Timestamp: 2025-04-23T08:51:57.756Z
Learning: In the webknossos codebase, classes extending `FoxImplicits` have access to an implicit conversion from `Option[A]` to `Fox[A]`, where `None` is converted to an empty Fox that fails gracefully in for-comprehensions.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8236
File: webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/precomputed/NeuroglancerPrecomputedShardingUtils.scala:23-28
Timestamp: 2025-04-25T11:06:13.275Z
Learning: AlfuCache in the WebKnossos codebase has default configured TTL (time-to-live) and size limits, providing automatic protection against unbounded memory growth.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
CHANGELOG.released.md (1)
undefined
<retrieved_learning>
Learnt from: MichaelBuessemeyer
PR: #8221
File: CHANGELOG.unreleased.md:14-14
Timestamp: 2024-11-25T14:52:23.201Z
Learning: For the webknossos project, changelog entries in CHANGELOG.unreleased.md should be concise and avoid unnecessary detailed explanations.
</retrieved_learning>
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (1)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.865Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
🪛 LanguageTool
MIGRATIONS.released.md
[grammar] ~10-~10: Use proper spacing conventions.
Context: ...eases/tag/25.07.0) - 2025-07-01 Commits - The default thread pool size was increas...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~12-~12: Use proper spacing conventions.
Context: ...ed slightly more memory because of this. ### Postgres Evolutions - [134-dataset-layer...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~16-~16: Use proper spacing conventions.
Context: ....sql) - 135-neuroglancer-attachment.sql ## [25.06.2](https://github.com/scalablemind...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
🔇 Additional comments (16)
MIGRATIONS.released.md (1)
9-17: LGTM - Well-documented release entry.The migration entry properly documents the thread pool size increase and new Postgres evolutions. The note about potential memory usage increase is helpful for administrators.
Minor spacing improvements can be made based on static analysis feedback, but these are cosmetic.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
365-376: Excellent timing instrumentation implementation.The timing code correctly uses the Fox monad with
<-for the initial timing capture and appropriately logs elapsed time at key checkpoints without altering the existing control flow.This aligns well with the performance monitoring improvements across the agglomerate ID retrieval pipeline.
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/EditableMappingController.scala (1)
74-89: Consistent and well-implemented timing instrumentation.The timing implementation follows the same pattern as the datastore controller, properly using Fox monads and logging at appropriate checkpoints. The
log = trueparameter enables detailed performance logging in the service layer.The empty lines improve log readability, which is helpful for debugging performance issues.
util/src/main/scala/com/scalableminds/util/time/Instant.scala (1)
72-81: Smart refactoring that improves usability.The changes to
logSinceprovide two valuable improvements:
- Better chaining: Returning the current
Instantenables the timing patterns used throughout the codebase- Conditional logging: The
silentIfBelowparameter prevents log noise for quick operationsThe refactoring maintains backward compatibility while enabling the timing instrumentation patterns used in the datastore and tracingstore layers.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/Hdf5AgglomerateService.scala (4)
5-5: LGTM: Clean import additions for instrumentation.The imports for
Instant,LazyLogging, andDurationIntare properly added to support the timing and logging functionality introduced in this PR.Also applies to: 23-23, 30-30
32-32: LGTM: Proper LazyLogging integration.Adding
LazyLoggingtrait to enable logging capabilities is correctly implemented.
116-123: LGTM: Well-implemented timing instrumentation.The timing implementation follows good practices:
- Uses
Instant.nowfor precise timing- Conditional logging with
silentIfBelowthreshold prevents noise- Includes meaningful context (segment ID and cache class)
- Non-intrusive addition that doesn't affect core logic
The 1 millisecond threshold is appropriate for filtering out very fast operations.
304-304: LGTM: Informative cache type logging.The logging messages clearly indicate which cache implementation is being used (cumsum-based vs standard block cache), which will be valuable for debugging and performance analysis.
Also applies to: 307-307
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/TSRemoteDatastoreClient.scala (3)
8-8: LGTM: Required import for timing functionality.Proper addition of the
Instantimport to support the timing instrumentation.
82-84: LGTM: Excellent early return optimization.The early return for empty input lists is a smart optimization that avoids unnecessary RPC calls. The addition of the optional
logparameter maintains backward compatibility while enabling detailed logging when needed.
85-97: LGTM: Comprehensive timing instrumentation.The implementation effectively captures timing for both key operations:
- Remote layer URI resolution
- RPC call execution
The conditional logging based on the
logparameter allows for detailed performance analysis without affecting normal operations. The use ofInstant.nowFoxfor Fox-compatible timing is appropriate for the async context.webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingService.scala (5)
271-271: LGTM: Debug prefix for targeted logging.The debug ID prefix allows for selective detailed logging of specific tracing operations, which is useful for debugging without overwhelming the logs.
232-232: LGTM: Timing instrumentation for chunk retrieval.The timing measurement around
getSegmentToAgglomerateChunkFilteredprovides visibility into the performance of chunk-based operations, including the number of chunks processed.Also applies to: 235-235
275-275: LGTM: Conditional fossil DB call timing.The selective logging based on the debug prefix prevents log noise while still capturing timing for specific operations. The fossil DB call timing will help identify performance bottlenecks in the data retrieval layer.
Also applies to: 278-279
285-304: LGTM: Comprehensive method instrumentation.The addition of the
logparameter and timing instrumentation provides excellent observability:
- Times the editable mapping retrieval step
- Times the base mapping retrieval step
- Uses conditional logging based on both the
logparameter and debug prefix- Maintains backward compatibility with default parameter
The tracing ID is included in log messages for correlation.
359-368: LGTM: Consistent parameter threading.The addition of the
logparameter togetBaseSegmentToAgglomerateand its proper threading to the downstreamgetAgglomerateIdsForSegmentIdscall maintains consistency across the instrumentation changes.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)