Development: Check bean instantiations on startup - #11065
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/core/config/BeanInstantiationTracer.java (6)
34-37: Avoid magic numbers; externalize thresholds to prevent drift with GitHub ActionThe comment notes manual sync with the workflow. Prefer sourcing thresholds from system properties or config to avoid drift.
Apply this minimal change to read from a system property with sane defaults:
- private static final int STARTUP_MAX_DEPENDENCY_CHAIN_THRESHOLD = 9; + private static final int STARTUP_MAX_DEPENDENCY_CHAIN_THRESHOLD = + Integer.getInteger("artemis.startup.maxBeanChain", 9); - private static final int DEFERRED_INIT_MAX_DEPENDENCY_CHAIN_THRESHOLD = 16; + private static final int DEFERRED_INIT_MAX_DEPENDENCY_CHAIN_THRESHOLD = + Integer.getInteger("artemis.deferred.maxBeanChain", 16);You can then set these via the GitHub Action Java args to ensure a single source of truth. Would you like a follow-up patch to wire these from the workflow?
65-77: Race between max depth and longestChain updates; ensure atomicity of the pair
maxCallStackSizeandlongestChaincan get out of sync under concurrency. A thread may fail the CAS yet still updatelongestChain, or two threads can interleave causing the chain not to match the recorded max.Make the update linearizable by coupling the successful CAS with the chain update:
- int prevMax = maxCallStackSize.get(); - if (depth > prevMax && maxCallStackSize.compareAndSet(prevMax, depth)) { - longestChain.set(new ArrayList<>(stack)); - } + int prev; + boolean updated = false; + do { + prev = maxCallStackSize.get(); + if (depth <= prev) { + break; + } + updated = maxCallStackSize.compareAndSet(prev, depth); + } + while (!updated); + if (updated) { + longestChain.set(new ArrayList<>(stack)); + }
94-97: Nit: spelling and punctuation in JavadocMinor doc polish.
- * Prints the bean instantiation graph to a DOT file that can be visualized on <a href="http://www.webgraphviz.com/">GraphViz</a> when the application starts up. + * Prints the bean instantiation graph to a DOT file that can be visualized on <a href="http://www.webgraphviz.com/">Graphviz</a> when the application starts up. - * This is useful for debugging and performance improvements, but should not be enabled in production environments + * This is useful for debugging and performance improvements, but should not be enabled in production environments.
106-106: Emit summary metric at INFO to facilitate GitHub Action parsingIf the Action parses logs, relying on DEBUG may hide the metric depending on log level config in CI.
- log.debug("Bean instantiation graph exported to startupBeans.dot ({} edges, longest dependency chain length: {})", edges.size(), longestChain.get().size()); + log.info("Bean instantiation graph exported to startupBeans.dot ({} edges, longest dependency chain length: {})", edges.size(), longestChain.get().size());If CI already enables DEBUG for this class, feel free to skip. Otherwise, consider promoting just the summary lines to INFO.
151-151: Nit: remove trailing space in log message placeholderCosmetic cleanup.
- log.error("Failed to write {} ", filename, e); + log.error("Failed to write {}", filename, e);
154-154: Consider promoting deferred max summary to INFO as wellSame rationale as Line 106 for CI parsing.
- log.debug("Maximum dependency chain length during deferred eager init: {}", maxCallStackSize.get()); + log.info("Maximum dependency chain length during deferred eager init: {}", maxCallStackSize.get());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/bean-instantiations.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/core/config/BeanInstantiationTracer.java(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/**/*.java
⚙️ CodeRabbit Configuration File
naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
Files:
src/main/java/de/tum/cit/aet/artemis/core/config/BeanInstantiationTracer.java
🧠 Learnings (2)
📚 Learning: 2025-08-04T11:27:40.470Z
Learnt from: tobias-lippert
PR: ls1intum/Artemis#11252
File: src/main/java/de/tum/cit/aet/artemis/core/service/telemetry/TelemetryService.java:45-45
Timestamp: 2025-08-04T11:27:40.470Z
Learning: In the Artemis application, there's a custom DeferredEagerBeanInitializer component that forces initialization of all lazy singleton beans after the FullStartupEvent is published. This allows beans to be marked Lazy for performance during startup while ensuring they're still initialized before serving requests. The PostConstruct methods on lazy beans will execute when the DeferredEagerBeanInitializer calls context.getBean(name) on them.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/core/config/BeanInstantiationTracer.java
📚 Learning: 2025-06-15T14:14:33.083Z
Learnt from: tobias-lippert
PR: ls1intum/Artemis#10951
File: src/main/java/de/tum/cit/aet/artemis/lti/config/DistributedStateAuthorizationRequestRepository.java:26-26
Timestamp: 2025-06-15T14:14:33.083Z
Learning: In the Artemis application, FullStartupEvent is published as the last event in the startup sequence, and lazy beans that need to respond to this event are guaranteed to be instantiated before or during its publication, so EventListener(FullStartupEvent.class) methods on Lazy beans will execute properly.
Applied to files:
src/main/java/de/tum/cit/aet/artemis/core/config/BeanInstantiationTracer.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: bean-instantiation-check
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/core/config/BeanInstantiationTracer.java (1)
130-149: Deferred chains export: good coverage and DOT generationThe new listener cleanly exports only offending deferred chains and logs them with lengths. Resource handling and iteration are correct.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
ahmetsenturk
left a comment
ahmetsenturk
left a comment
There was a problem hiding this comment.
code lgtm and output makes sense 👍
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
github-project-automation
Bot
moved this from Ready For Review
to Merged
in Artemis Development
5 participants
Checklist
General
Server
Motivation and Context
In the last couple of weeks we merged different PRs targeting the startup performance of the Artemis server application.
If we do not check these values for every PR performance regressions might be introduced without noticing it.
Description
Added a github action that checks the number of instantiated beans on startup and the longest chain length of bean instantiations. Additionally, we check the longest chain length after the deferred eager initialization
Steps for Testing
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Summary by CodeRabbit
New Features
Bug Fixes
Chores