Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closes #1280 - Closing inspectIT class loader when the agent is shutting down #1281

Merged
merged 6 commits into from
Feb 24, 2022

Conversation

mariusoe
Copy link
Member

@mariusoe mariusoe commented Jan 20, 2022

closes #1280


This change is Reviewable

@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #1281 (2273a47) into master (d37a7dc) will decrease coverage by 2.31%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1281      +/-   ##
============================================
- Coverage     83.00%   80.70%   -2.31%     
- Complexity     1736     2026     +290     
============================================
  Files           174      204      +30     
  Lines          5366     6466    +1100     
  Branches        650      769     +119     
============================================
+ Hits           4454     5218     +764     
- Misses          675      957     +282     
- Partials        237      291      +54     
Impacted Files Coverage Δ
...spectit/ocelot/core/utils/WeakMethodReference.java 0.00% <0.00%> (-77.27%) ⬇️
...strap/correlation/noop/NoopLogTraceCorrelator.java 28.57% <0.00%> (-42.86%) ⬇️
...ial/ScheduledExecutorContextPropagationSensor.java 82.05% <0.00%> (-12.39%) ⬇️
...in/java/rocks/inspectit/ocelot/core/AgentImpl.java 63.27% <0.00%> (-10.54%) ⬇️
...pectit/ocelot/config/loaders/ConfigFileLoader.java 82.61% <0.00%> (-8.70%) ⬇️
...lot/bootstrap/context/noop/NoopContextManager.java 10.00% <0.00%> (-6.67%) ⬇️
...core/instrumentation/InstrumentationTriggerer.java 85.19% <0.00%> (-5.63%) ⬇️
...t/ocelot/config/validation/PropertyPathHelper.java 75.56% <0.00%> (-5.06%) ⬇️
...rocks/inspectit/ocelot/config/utils/CaseUtils.java 90.48% <0.00%> (-4.98%) ⬇️
...t/ocelot/core/instrumentation/hook/MethodHook.java 95.35% <0.00%> (-4.65%) ⬇️
... and 96 more

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 7 files at r1.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @heiko-holz and @mariusoe)


inspectit-ocelot-agent/src/main/java/rocks/inspectit/ocelot/agent/AgentMain.java, line 61 at r1 (raw file):

        try {
            if (loadOpenCensusToBootstrap) {
                Path ocJarFile = copyResourceToTempJarFile(OPENCENSUS_FAT_JAR_PATH, "opencensus-fat-");

wouldn't it be more suitable to have private static final String OPENCENSUS_FAT_JAR_TEMP_PREFIX variables (also for bootstrap and core?)


inspectit-ocelot-agent/src/main/java/rocks/inspectit/ocelot/agent/AgentMain.java, line 102 at r1 (raw file):

        if (includeOpenCensus) {
            Path ocJarFile = copyResourceToTempJarFile(OPENCENSUS_FAT_JAR_PATH, "ocelot-oc-fat-");

why does this prefix differ to the one used above for OPENCENSUS_FAT_JAR_PATH?

@heiko-holz heiko-holz assigned heiko-holz and mariusoe and unassigned mariusoe Jan 31, 2022
Copy link
Member Author

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @heiko-holz)


inspectit-ocelot-agent/src/main/java/rocks/inspectit/ocelot/agent/AgentMain.java, line 61 at r1 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

wouldn't it be more suitable to have private static final String OPENCENSUS_FAT_JAR_TEMP_PREFIX variables (also for bootstrap and core?)

Done.


inspectit-ocelot-agent/src/main/java/rocks/inspectit/ocelot/agent/AgentMain.java, line 102 at r1 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

why does this prefix differ to the one used above for OPENCENSUS_FAT_JAR_PATH?

Done.

Copy link
Member Author

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 7 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @heiko-holz)

Copy link
Contributor

@heiko-holz heiko-holz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mariusoe)

@heiko-holz heiko-holz merged commit 4010621 into inspectIT:master Feb 24, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Close inspectIT classloader when its context is closed
2 participants