Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Closes #377 - Prevent recursive method hook execution #1262

Merged
merged 6 commits into from
Feb 17, 2022

Conversation

mariusoe
Copy link
Member

@mariusoe mariusoe commented Dec 23, 2021

closes #377


This change is Reviewable

@mariusoe mariusoe force-pushed the feature/377-prevent-recursion branch from dff5303 to 7eb46b3 Compare January 20, 2022 10:19
@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #1262 (18d9229) into master (d37a7dc) will decrease coverage by 2.21%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1262      +/-   ##
============================================
- Coverage     83.00%   80.79%   -2.21%     
- Complexity     1736     2026     +290     
============================================
  Files           174      204      +30     
  Lines          5366     6460    +1094     
  Branches        650      769     +119     
============================================
+ Hits           4454     5219     +765     
- Misses          675      950     +275     
- 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%) ⬇️
...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%) ⬇️
...del/metrics/definition/ViewDefinitionSettings.java 45.45% <0.00%> (-4.55%) ⬇️
... and 92 more

@mariusoe mariusoe marked this pull request as ready for review January 20, 2022 10:34
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 3 of 4 files at r1.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on @heiko-holz and @mariusoe)


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/hook/HookManagerTest.java, line 80 at r1 (raw file):

            // set recursion gate
            HookManager.RECURSION_GATE.set(true);

Is it feasible to also write a "real scenario" test case for which the RECURSION_GATE is not set manually but by the "poorly chosen instrumentation"?
As I am too far away from the implementation, I fail to grasp the example given in #377

@heiko-holz heiko-holz self-assigned this Jan 31, 2022
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 r2.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @heiko-holz and @mariusoe)

@heiko-holz heiko-holz assigned mariusoe and unassigned mariusoe Feb 3, 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.

Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @heiko-holz)


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/hook/HookManagerTest.java, line 80 at r1 (raw file):

Previously, heiko-holz (Heiko Holz) wrote…

Is it feasible to also write a "real scenario" test case for which the RECURSION_GATE is not set manually but by the "poorly chosen instrumentation"?
As I am too far away from the implementation, I fail to grasp the example given in #377

I've added a system test as well including an instrumentation configuration to cover this case (where an instrumentation calls an instrumented method within its instrumentation). Maybe it is more clear now.

…InstrumentationTest.java (and updating recursiveInstrumentation.yml)
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 4 files at r1, 1 of 2 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mariusoe)


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/hook/HookManagerTest.java, line 80 at r1 (raw file):

Previously, mariusoe (Marius Oehler) wrote…

I've added a system test as well including an instrumentation configuration to cover this case (where an instrumentation calls an instrumented method within its instrumentation). Maybe it is more clear now.

Yes, the tests phenomenally explain the issue :)

@mariusoe mariusoe merged commit fdfb018 into inspectIT:master Feb 17, 2022
@mariusoe mariusoe deleted the feature/377-prevent-recursion branch February 17, 2022 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent instrumentation recursion
2 participants