Skip to content

Conversation

@manuel-alvarez-alvarez
Copy link
Member

@manuel-alvarez-alvarez manuel-alvarez-alvarez commented Aug 9, 2022

What Does This Do

This PR contains the basics to do Call Site Instrumentation in the tracer. It defines a new type of instrumenter datadog.trace.agent.tooling.bytebuddy.csi.CallSiteInstrumenter that is able to register instances of datadog.trace.agent.tooling.csi.CallSiteAdvice to perform the instrumentation via ASM.

Motivation

Instrumentations related to IAST often affect core parts of the JDK (String, StringBuilder...) where using callee instrumentation (by default in ByteBuddy) is not good enough for performance. Call site instrumentation focuses on the calls to the instrumented methods enabling the use of inclusion/exclusion lists to fine tune where to instrument.

Additional Notes

Following this PR comes [IAST] Call Site Instrumentation plugin #3729 where advices can be autogenerated by a gradle plugin. It also showcases an example on how to use CSI in the tracer.

Benchmark results

The main goal of this results are to validate that call site instrumentation is feasible in terms of startup time of an application:

Benchmark Mode Cnt Score Error Units
CallSiteBenchmark.callSite ss 10 1572.961 ± 36.189 ms/op
CallSiteBenchmark.callee ss 10 1551.316 ± 31.888 ms/op
CallSiteBenchmark.none ss 10 1337.549 ± 52.882 ms/op

@manuel-alvarez-alvarez manuel-alvarez-alvarez added comp: asm iast Application Security Management (IAST) tag: do not merge Do not merge changes in-progress labels Aug 9, 2022
@manuel-alvarez-alvarez manuel-alvarez-alvarez changed the title 🚧 👷 🏗️ Initial version with CSI support 🚧 👷 🏗️ Call Site Instrumentation support Aug 9, 2022
@manuel-alvarez-alvarez manuel-alvarez-alvarez force-pushed the malvarez/call-site-instrumentation branch 2 times, most recently from 1ff7c7a to 14e0381 Compare August 9, 2022 16:59
@jandro996 jandro996 marked this pull request as ready for review August 10, 2022 08:21
@jandro996 jandro996 requested a review from a team as a code owner August 10, 2022 08:21
@manuel-alvarez-alvarez manuel-alvarez-alvarez force-pushed the malvarez/call-site-instrumentation branch from 14e0381 to abe78a6 Compare August 10, 2022 08:31
@manuel-alvarez-alvarez manuel-alvarez-alvarez marked this pull request as draft August 10, 2022 08:46
@manuel-alvarez-alvarez manuel-alvarez-alvarez force-pushed the malvarez/call-site-instrumentation branch from abe78a6 to c0fa4cc Compare August 10, 2022 10:01
@manuel-alvarez-alvarez manuel-alvarez-alvarez force-pushed the malvarez/call-site-instrumentation branch 5 times, most recently from 7c6708e to 3c7a72a Compare August 10, 2022 16:56
@manuel-alvarez-alvarez manuel-alvarez-alvarez changed the title 🚧 👷 🏗️ Call Site Instrumentation support [WIP] Call Site Instrumentation support Aug 11, 2022
@manuel-alvarez-alvarez manuel-alvarez-alvarez changed the title [WIP] Call Site Instrumentation support [WIP][IAST] Call Site Instrumentation support Aug 11, 2022
@manuel-alvarez-alvarez manuel-alvarez-alvarez force-pushed the malvarez/call-site-instrumentation branch from 3c7a72a to 093220c Compare August 11, 2022 17:43
@manuel-alvarez-alvarez manuel-alvarez-alvarez changed the title [WIP][IAST] Call Site Instrumentation support [IAST] Call Site Instrumentation support Aug 11, 2022
@manuel-alvarez-alvarez manuel-alvarez-alvarez marked this pull request as ready for review August 11, 2022 18:04
@manuel-alvarez-alvarez manuel-alvarez-alvarez force-pushed the malvarez/call-site-instrumentation branch 3 times, most recently from fc95339 to 6213929 Compare August 19, 2022 15:05
@manuel-alvarez-alvarez manuel-alvarez-alvarez force-pushed the malvarez/call-site-instrumentation branch from 6213929 to 80f3fab Compare August 22, 2022 08:10
@manuel-alvarez-alvarez manuel-alvarez-alvarez force-pushed the malvarez/call-site-instrumentation branch 6 times, most recently from 9826dc7 to ba0d10f Compare August 24, 2022 09:19
@manuel-alvarez-alvarez manuel-alvarez-alvarez force-pushed the malvarez/call-site-instrumentation branch 2 times, most recently from ea531af to 8afa126 Compare September 2, 2022 10:37
@manuel-alvarez-alvarez manuel-alvarez-alvarez force-pushed the malvarez/call-site-instrumentation branch from 8afa126 to 7021e24 Compare September 5, 2022 07:30
Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

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

Great PR!

There is only one thing that I think needs fixing in some way before merging.

I was a bit confused by the benchmarks, and it feels very counter intuitive that there should be any performance difference between callSite and callee in this case, so I had to poke at them a bit, and the response differs. The callSite one gets back Hello! [Transformed] and the callee one gets back Hello! [Transformed] [Transformed]. So we're not comparing the same things.

@manuel-alvarez-alvarez
Copy link
Member Author

Great PR!

There is only one thing that I think needs fixing in some way before merging.

I was a bit confused by the benchmarks, and it feels very counter intuitive that there should be any performance difference between callSite and callee in this case, so I had to poke at them a bit, and the response differs. The callSite one gets back Hello! [Transformed] and the callee one gets back Hello! [Transformed] [Transformed]. So we're not comparing the same things.

@bantonsson, first of all thanks for the feedback 😃, much appreciated!

My idea with the benchmark is to validate that doing call site instrumentation is feasible in terms of startup time as long as there's a proper exclusion list.

In this particular case I wanted to showcase the instrumentation of javax.servlet.http.HttpServlet#getParameter as it's something we need for IAST. It's true that when using callee, due to wrappers/facades/... you end up being executed a few times in the stack of a request (that's why we got the double [Transformed] response). I've changed to instrument only the real implementation of the interface (in this particular case it's the apache connector).

Let me know if I should do something else about it.

@manuel-alvarez-alvarez manuel-alvarez-alvarez merged commit d45b683 into master Sep 6, 2022
@manuel-alvarez-alvarez manuel-alvarez-alvarez deleted the malvarez/call-site-instrumentation branch September 6, 2022 11:42
@github-actions github-actions bot added this to the 0.109.0 milestone Sep 6, 2022
@bantonsson bantonsson modified the milestones: 0.108.2, 0.109.0 Sep 15, 2022
@smola smola added the tag: no release notes Changes to exclude from release notes label Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: asm iast Application Security Management (IAST) tag: no release notes Changes to exclude from release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants