-
Notifications
You must be signed in to change notification settings - Fork 312
Add RUM SDK injection for servlet based web servers #9110
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
Conversation
RUM injection remote config is not dynamic as other products
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left few questions.
Looking good to me 👍
...quest-3/src/main/java/datadog/trace/instrumentation/servlet3/WrappedServletOutputStream.java
Outdated
Show resolved
Hide resolved
...quest-3/src/main/java/datadog/trace/instrumentation/servlet3/WrappedServletOutputStream.java
Outdated
Show resolved
Hide resolved
...st-5/src/main/java/datadog/trace/instrumentation/servlet5/RumHttpServletResponseWrapper.java
Show resolved
Hide resolved
...st-5/src/main/java/datadog/trace/instrumentation/servlet5/RumHttpServletResponseWrapper.java
Outdated
Show resolved
Hide resolved
...st-3/src/main/java/datadog/trace/instrumentation/servlet3/RumHttpServletResponseWrapper.java
Outdated
Show resolved
Hide resolved
...st-3/src/main/java/datadog/trace/instrumentation/servlet3/RumHttpServletResponseWrapper.java
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/api/rum/RumInjector.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/api/rum/RumInjector.java
Outdated
Show resolved
Hide resolved
internal-api/src/main/java/datadog/trace/api/rum/RumInjectorConfig.java
Outdated
Show resolved
Hide resolved
.../src/main/java/datadog/trace/bootstrap/instrumentation/buffer/InjectingPipeOutputStream.java
Outdated
Show resolved
Hide resolved
found = true; | ||
drain(); | ||
out.write(b, off, idx); | ||
out.write(contentToInject); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How resilient is this code to a misbehaving out
stream?
In other words if out.write
suddenly threw an IOException
in the middle of one of our buffering methods, would the state kept here remain consistent - so that if the application code attempted to write again, everything would resume as normal...
One way to confirm this would be to run tests where the out
stream being wrapped threw an IOException
on every other call. Ideally you should end up with the same behaviour from the application's perspective with or without this wrapper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not today. I need kind of checkpoint the action on the buffer in order to make it resilient
...st-5/src/main/java/datadog/trace/instrumentation/servlet5/RumHttpServletResponseWrapper.java
Show resolved
Hide resolved
...st-5/src/main/java/datadog/trace/instrumentation/servlet5/RumHttpServletResponseWrapper.java
Show resolved
Hide resolved
...st-5/src/main/java/datadog/trace/instrumentation/servlet5/RumHttpServletResponseWrapper.java
Show resolved
Hide resolved
return super.getWriter(); | ||
} | ||
if (printWriter == null) { | ||
printWriter = new PrintWriter(getOutputStream()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could result in changed behaviour if the underlying servlet framework has a getWriter()
implementation that does more than just delegate to getOutputStream()
We could add a WrappedPrintWriter
and pass it the result of super.getWriter()
- while it means more code IMHO it would be safer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I had the same concern indeed. I wanted to safe few wrapping but I'm also incline to do that
...st-3/src/main/java/datadog/trace/instrumentation/servlet3/RumHttpServletResponseWrapper.java
Show resolved
Hide resolved
...st-3/src/main/java/datadog/trace/instrumentation/servlet3/RumHttpServletResponseWrapper.java
Show resolved
Hide resolved
...st-3/src/main/java/datadog/trace/instrumentation/servlet3/RumHttpServletResponseWrapper.java
Show resolved
Hide resolved
return super.getWriter(); | ||
} | ||
if (printWriter == null) { | ||
printWriter = new PrintWriter(getOutputStream()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in the servlet-5
instrumentation, I think a WrappedPrintWriter
approach would be safer.
.../src/main/java/datadog/trace/bootstrap/instrumentation/buffer/InjectingPipeOutputStream.java
Show resolved
Hide resolved
} | ||
} else { | ||
// use slow path because the length to write is small and within the lookbehind buffer size | ||
super.write(b, off, len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This assumes that super.write(b, off, len)
will repeatedly call write(int b)
on this instance.
If it instead calls a shared internal method used by both write
methods then we wouldn't detect the marker.
EDIT: to be sure we probably want to call write(int b)
ourselves in a loop...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assumed that because of FilterOutputStream
does it but I'm not sure that the implementation will stay stable throughout the jdk versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some things to be confirmed:
- Is it feasible to add a
PrintWriter
wrapper? - How does
InjectingPipeOutputStream
behave with an unreliable stream? - Slow path should repeatedly call
write(int b)
rather than assume thatsuper.write(b, off, len)
will do it - Can we document somewhere the effect of RUM injection on
Content-Length
headers (i.e. they won't appear because the size cannot be pre-determined)
Since this is under an opt-in feature flag I'm ok with this being merged while these issues are investigated
| Package | Type | Package file | Manager | Update | Change | |---|---|---|---|---|---| | [com.google.errorprone:error_prone_annotations](https://errorprone.info) ([source](https://github.com/google/error-prone)) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `2.39.0` -> `2.40.0` | | [org.apache.commons:commons-lang3](https://commons.apache.org/proper/commons-lang/) ([source](https://gitbox.apache.org/repos/asf/commons-lang.git)) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `3.17.0` -> `3.18.0` | | [org.jetbrains.kotlinx.binary-compatibility-validator](https://github.com/Kotlin/binary-compatibility-validator) | plugin | misk/gradle/libs.versions.toml | gradle | patch | `0.18.0` -> `0.18.1` | | [com.datadoghq:dd-trace-api](https://github.com/datadog/dd-trace-java) | dependencies | misk/gradle/libs.versions.toml | gradle | minor | `1.50.1` -> `1.51.0` | | [software.amazon.awssdk:sdk-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.77` -> `2.31.78` | | [software.amazon.awssdk:sqs](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.77` -> `2.31.78` | | [software.amazon.awssdk:dynamodb-enhanced](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.77` -> `2.31.78` | | [software.amazon.awssdk:dynamodb](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.77` -> `2.31.78` | | [software.amazon.awssdk:aws-core](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.77` -> `2.31.78` | | [software.amazon.awssdk:bom](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.77` -> `2.31.78` | | [software.amazon.awssdk:auth](https://aws.amazon.com/sdkforjava) | dependencies | misk/gradle/libs.versions.toml | gradle | patch | `2.31.77` -> `2.31.78` | --- ### Release Notes <details> <summary>google/error-prone (com.google.errorprone:error_prone_annotations)</summary> ### [`v2.40.0`](https://github.com/google/error-prone/releases/tag/v2.40.0): Error Prone 2.40.0 Changes: - Bug fixes and improvements - Releases (including snapshots) have migrated from [OSSRH to the Central Publisher Portal](https://central.sonatype.org/pages/ossrh-eol/#process-to-migrate) Full changelog: google/error-prone@v2.39.0...v2.40.0 </details> <details> <summary>Kotlin/binary-compatibility-validator (org.jetbrains.kotlinx.binary-compatibility-validator)</summary> ### [`v0.18.1`](https://github.com/Kotlin/binary-compatibility-validator/releases/tag/0.18.1) [Compare Source](Kotlin/binary-compatibility-validator@0.18.0...0.18.1) #### What's Changed - Fixed a bug preventing use of cross-compilation support during KLIB dump validation \[[#​304](https://github.com/Kotlin/binary-compatibility-validator/issues/304)]\[[#​306](https://github.com/Kotlin/binary-compatibility-validator/issues/306)] </details> <details> <summary>datadog/dd-trace-java (com.datadoghq:dd-trace-api)</summary> ### [`v1.51.0`](https://github.com/DataDog/dd-trace-java/releases/tag/v1.51.0): 1.51.0 ### Components #### Application Security Management (IAST) - 🐛 Fix verify error when ctor params are used after a call site ([#​9083](DataDog/dd-trace-java#9083) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) - 🐛 Limit the maximum size of the location path in IAST vulnerabilities ([#​9028](DataDog/dd-trace-java#9028) - [@​jandro996](https://github.com/jandro996)) - 🐛 Fix IAST gRPC handler with null superclass ([#​8984](DataDog/dd-trace-java#8984) - [@​smola](https://github.com/smola)) - ✨ Optimize IAST Vulnerability Detection ([#​8885](DataDog/dd-trace-java#8885) - [@​jandro996](https://github.com/jandro996)) #### Application Security Management (WAF) - ✨ Upgrade libddwaf-java to 15.0.0 ([#​9022](DataDog/dd-trace-java#9022) - [@​sezen-datadog](https://github.com/sezen-datadog)) - ✨ Extract RestEasy json body response schemas ([#​9015](DataDog/dd-trace-java#9015) - [@​jandro996](https://github.com/jandro996)) - ✨ Extract Jersey json body response schemas ([#​9014](DataDog/dd-trace-java#9014) - [@​jandro996](https://github.com/jandro996)) - ✨ Extract Ratpack json body response schemas ([#​9013](DataDog/dd-trace-java#9013) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) - ✨ Enable API Security by default and make it lazy loading ([#​9009](DataDog/dd-trace-java#9009) - [@​smola](https://github.com/smola)) - ✨ Extract Vert.x json body response schemas ([#​9001](DataDog/dd-trace-java#9001) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) - ✨ Extract Play json body response schemas ([#​8995](DataDog/dd-trace-java#8995) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) - 🐛 Fix Jackson nodes introspection for request/response schema extraction ([#​8980](DataDog/dd-trace-java#8980) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) - ✨ Extract Spring json body response schemas ([#​8938](DataDog/dd-trace-java#8938) - [@​sezen-datadog](https://github.com/sezen-datadog)) - ✨ Default obfuscation regexp update ([#​8937](DataDog/dd-trace-java#8937) - [@​sezen-datadog](https://github.com/sezen-datadog)) #### Build & Tooling - ✨ Cancel GitLab running pipeline on new PR push ([#​9023](DataDog/dd-trace-java#9023) - [@​PerfectSlayer](https://github.com/PerfectSlayer)) - ✨ Migrate publishing to Maven Central Portal ([#​8807](DataDog/dd-trace-java#8807) - [@​sarahchen6](https://github.com/sarahchen6)) #### Continuous Integration Visibility - 🐛 Fix Test Optimization to work with JDK 24 ([#​9114](DataDog/dd-trace-java#9114) - [@​nikita-tkachenko-datadog](https://github.com/nikita-tkachenko-datadog)) - ✨ Add repo root as safe directory on git client creation ([#​9033](DataDog/dd-trace-java#9033) - [@​daniel-mohedano](https://github.com/daniel-mohedano)) - ✨ Add PR number tag and improve PR information building ([#​8990](DataDog/dd-trace-java#8990) - [@​daniel-mohedano](https://github.com/daniel-mohedano)) - ✨ Update impacted tests logic ([#​8923](DataDog/dd-trace-java#8923) - [@​daniel-mohedano](https://github.com/daniel-mohedano)) #### Data Streams Monitoring - 🧹 Clean up DSM context injection ([#​8776](DataDog/dd-trace-java#8776) - [@​PerfectSlayer](https://github.com/PerfectSlayer)) #### Database Monitoring - 🐛 Set trace\_injected in try block ([#​9025](DataDog/dd-trace-java#9025) - [@​natashadada](https://github.com/natashadada)) #### Dynamic Instrumentation - 🐛 Add source file tracking enable option ([#​9115](DataDog/dd-trace-java#9115) - [@​jpbempel](https://github.com/jpbempel)) - ✨ Add java.util.Date support ([#​9111](DataDog/dd-trace-java#9111) - [@​jpbempel](https://github.com/jpbempel)) - ✨ Update file probe format ([#​9047](DataDog/dd-trace-java#9047) - [@​jpbempel](https://github.com/jpbempel)) - ✨ add safe local var hoisting ([#​9034](DataDog/dd-trace-java#9034) - [@​jpbempel](https://github.com/jpbempel)) - 🧹 Add new config for debugger upload interval ([#​8959](DataDog/dd-trace-java#8959) - [@​jpbempel](https://github.com/jpbempel)) - ✨ Enable Code Origin with Dynamic instrumentation ([#​8940](DataDog/dd-trace-java#8940) - [@​jpbempel](https://github.com/jpbempel)) #### ML Observability (LLMObs) - 💡 LLM Observability SDK ([#​8781](DataDog/dd-trace-java#8781) - [@​gary-huang](https://github.com/gary-huang), [@​nayeem-kamal](https://github.com/nayeem-kamal)) #### Metrics - 🐛 Ensure client stat reporter is started when the agent is not available at bootstrap ([#​9082](DataDog/dd-trace-java#9082) - [@​amarziali](https://github.com/amarziali)) - ✨ Create metric: appsec.waf.config\_errors ([#​8394](DataDog/dd-trace-java#8394) - [@​sezen-datadog](https://github.com/sezen-datadog)) #### Platform components - ✨ Introduce environment component ([#​9071](DataDog/dd-trace-java#9071) - [@​PerfectSlayer](https://github.com/PerfectSlayer)) #### Profiling - 🐛 Remove annoying warning for smap event parsing ([#​9119](DataDog/dd-trace-java#9119) - [@​jbachorik](https://github.com/jbachorik)) - 🐛 Fix ByteCountingInputStream when reading past EOF ([#​8988](DataDog/dd-trace-java#8988) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) #### Realtime User Monitoring - ✨ Add RUM SDK injection for servlet based web servers ([#​9110](DataDog/dd-trace-java#9110) - [@​PerfectSlayer](https://github.com/PerfectSlayer) [@​amarziali](https://github.com/amarziali)) #### Telemetry - ✨ Update the config origin metric to match what it's mapping ([#​9045](DataDog/dd-trace-java#9045) - [@​sezen-datadog](https://github.com/sezen-datadog)) #### Testing - ✨ Add testing for latest stable version (JDK 24) ([#​8875](DataDog/dd-trace-java#8875) - [@​sarahchen6](https://github.com/sarahchen6)) #### Trace context propagation - 🐛 Fix bug with dropping baggage when `TracePropagationBehaviorExtract=IGNORE` ([#​9037](DataDog/dd-trace-java#9037) - [@​mhlidd](https://github.com/mhlidd)) - 🐛 Fix ArrayIndexOutOfBoundsException in PercentEscaper ([#​9032](DataDog/dd-trace-java#9032) - [@​mhlidd](https://github.com/mhlidd)) #### Tracer core - 🐛 Fix `Error` handling for trace interceptors ([#​9097](DataDog/dd-trace-java#9097) - [@​AlexeyKuznetsov-DD](https://github.com/AlexeyKuznetsov-DD)) - 💡 Add wildcard feature for `DD_TRACE_HEADER_TAGS` and enabling for Http Response headers ([#​9067](DataDog/dd-trace-java#9067) - [@​mhlidd](https://github.com/mhlidd)) #### Tracer public API - 💡 Add LLM Observability SDK ([#​8781](DataDog/dd-trace-java#8781) - [@​gary-huang](https://github.com/gary-huang)) ### Instrumentations #### Akka instrumentation - 🐛 Fix NPE in akka-http and pekko-http integrations ([#​9019](DataDog/dd-trace-java#9019) - [@​mcculls](https://github.com/mcculls)) #### Eclipse Vert.x instrumentation - ✨ Extract Vert.x json body response schemas ([#​9001](DataDog/dd-trace-java#9001) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) - ✨ Write http.route tag as soon as possible in vert.x ([#​8952](DataDog/dd-trace-java#8952) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) #### JAX-WS instrumentation - 💡⚠️ Enable jax-ws integration by default ([#​9030](DataDog/dd-trace-java#9030) - [@​bm1549](https://github.com/bm1549)) - ✨ Extract Jersey json body response schemas ([#​9014](DataDog/dd-trace-java#9014) - [@​jandro996](https://github.com/jandro996)) #### Mule instrumentation - 🐛 Propagate grizzly http span in filters if nothing is active ([#​9016](DataDog/dd-trace-java#9016) - [@​amarziali](https://github.com/amarziali)) #### Play Framework instrumentation - ✨ Extract Play json body response schemas ([#​8995](DataDog/dd-trace-java#8995) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) #### Ratpack instrumentation - ✨ Extract Ratpack json body response schemas ([#​9013](DataDog/dd-trace-java#9013) - [@​manuel-alvarez-alvarez](https://github.com/manuel-alvarez-alvarez)) #### Spring instrumentation - ✨ Extract Spring json body response schemas ([#​8938](DataDog/dd-trace-java#8938) - [@​sezen-datadog](https://github.com/sezen-datadog)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Never, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). GitOrigin-RevId: 649b690d4c9d7dcb572c457f0802b42b8e3e682e
What Does This Do
This PR introduces RUM SDK injection for servlet APIs 3, 4 and 5.
In addition to the RUM SDK injection itself, the PR also contains:
OutputStream
wrapper in charge of the SDK script injectionMotivation
Brings parity with ngnix and apache support.
Additional Notes
Co-authored by @amarziali and @PerfectSlayer
Contributor Checklist
type:
and (comp:
orinst:
) labels in addition to any usefull labelsclose
,fix
or any linking keywords when referencing an issue.Use
solves
instead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]