Skip to content

Add servlet 2.2 instrumentation - #376

Open
kaushal02 wants to merge 7 commits into
mainfrom
kaushal02/instrumentation-servlet-2.2
Open

Add servlet 2.2 instrumentation#376
kaushal02 wants to merge 7 commits into
mainfrom
kaushal02/instrumentation-servlet-2.2

Conversation

@kaushal02

@kaushal02kaushal02 commented Feb 3, 2023•
edited
Loading

Copy link
Copy Markdown

Description

Adding instrumentation for old servlet versions (2.2-3.0). This is used in boomi. The implementation is almost similar to servlet-3.0.

Testing

Tested with rest API in boomi which uses servlet-2.5 and able to instrument everything there except response headers.

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@kaushal02
kaushal02 marked this pull request as ready for review March 20, 2023 14:43
shashank11p
shashank11p reviewed Mar 23, 2023
View reviewed changes

@shashank11pshashank11p left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kaushal02 there are some TODOs and FIXMEs pending

Comment threadinstrumentation/servlet/servlet-2.2/build.gradle.kts Outdated
Comment thread...ent/instrumentation/hypertrace/servlet/v2_2/nowrapping/Servlet2AndFilterInstrumentation.java Outdated
Comment thread...ent/instrumentation/hypertrace/servlet/v2_2/nowrapping/Servlet2AndFilterInstrumentation.java Outdated
Comment thread...nstrumentation/hypertrace/servlet/v2_2/nowrapping/request/ServletRequestInstrumentation.java Outdated
Comment thread...entation/hypertrace/servlet/v2_2/nowrapping/response/ServletOutputStreamInstrumentation.java Outdated
.and(isPublic()),
ServletOutputStreamInstrumentation.class.getName() + "$OutputStream_writeByteArrOffset");

// close() is not instrumented due to some issue with Tomcat

@shashank11pshashank11pMar 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we instrument this method in 2.0? We should check this method in servlet-2.0 code

Comment thread...nstrumentation/hypertrace/servlet/v3_0/nowrapping/request/ServletRequestInstrumentation.java Outdated
Comment thread...pentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/request/Utils.java Outdated
Comment threadjavaagent/build.gradle.kts Outdated
@shashank11p

shashank11p commented Mar 23, 2023

Copy link
Copy Markdown
Contributor

@kaushal02 we should add tests as well

shashank11p
shashank11p reviewed Mar 23, 2023
View reviewed changes
Comment thread...pentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/request/Utils.java Outdated
Comment thread...ent/instrumentation/hypertrace/servlet/v2_2/nowrapping/Servlet2AndFilterInstrumentation.java Outdated
VirtualField.find(HttpServletRequest.class, StringMapSpanPair.class);

// capture response body
// TODO: capture response headers

@shashank11pshashank11pMar 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we able to capture response headers?

@kaushal02kaushal02Mar 23, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

we are capturing content-type header but other response headers are not getting captured, at least in case of Boomi

shashank11p
shashank11p reviewed Mar 23, 2023
View reviewed changes
Comment threadinstrumentation/servlet/servlet-2.2/build.gradle.kts
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shashank11pshashank11pshashank11p left review comments
@tyroneyutyroneyuAwaiting requested review from tyroneyu
@puneet-traceablepuneet-traceableAwaiting requested review from puneet-traceable
@ryanericsonryanericsonAwaiting requested review from ryanericson

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kaushal02@shashank11p