GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add servlet 2.2 instrumentation by kaushal02 · Pull Request #376 · hypertrace/javaagent

Conversation

kaushal02 commented Feb 3, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kaushal02 added 4 commits

February 1, 2023 16:46


          Add servlet 2.2 instrumentation

554d90f


          remove old warnings and add httpServletResponseInstrumentation

4a71a2d


          merge instrumentation files

d7eecf6


          remove comments and move from draft to ready

20edca1

kaushal02 marked this pull request as ready for review

March 20, 2023 14:43

kaushal02 requested review from JBAhire, puneet-traceable, ryanericson, shashank11p and tyroneyu and removed request for JBAhire

March 20, 2023 14:43

shashank11p reviewed

Mar 23, 2023

View reviewed changes

shashank11p left a comment

Copy link

Copy Markdown

Contributor

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.

@kaushal02 there are some TODOs and FIXMEs pending

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

instrumentation/servlet/servlet-2.2/build.gradle.kts Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

...ent/instrumentation/hypertrace/servlet/v2_2/nowrapping/Servlet2AndFilterInstrumentation.java Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

...ent/instrumentation/hypertrace/servlet/v2_2/nowrapping/Servlet2AndFilterInstrumentation.java Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

...nstrumentation/hypertrace/servlet/v2_2/nowrapping/request/ServletRequestInstrumentation.java Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

shashank11p Mar 23, 2023

Copy link

Copy Markdown

Contributor

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

...nstrumentation/hypertrace/servlet/v3_0/nowrapping/request/ServletRequestInstrumentation.java Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

...pentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/request/Utils.java Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

javaagent/build.gradle.kts Resolved

Uh oh!

There was an error while loading. Please reload this page.

shashank11p commented Mar 23, 2023

Copy link

Copy Markdown

Contributor

@kaushal02 we should add tests as well

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          PR comments

d0bd73e

shashank11p reviewed

Mar 23, 2023

View reviewed changes

Comment thread

...pentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/request/Utils.java Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

shashank11p Mar 23, 2023

Copy link

Copy Markdown

Contributor

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.

Are we able to capture response headers?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

kaushal02 Mar 23, 2023

Copy link

Copy Markdown

Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

shashank11p reviewed

Mar 23, 2023

View reviewed changes

Comment thread

instrumentation/servlet/servlet-2.2/build.gradle.kts Resolved

Uh oh!

There was an error while loading. Please reload this page.

kaushal02 added 2 commits

March 24, 2023 01:52


          fixed muzzle error, added tests WIP

16a5ca3


          fix jetty version

4aaada5

Reviewers

shashank11p

shashank11p left review comments

tyroneyu

Awaiting requested review from tyroneyu

puneet-traceable

Awaiting requested review from puneet-traceable

ryanericson

Awaiting requested review from ryanericson

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

kaushal02 commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Documentation

Uh oh!

shashank11p left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shashank11p Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shashank11p commented Mar 23, 2023

Uh oh!

Uh oh!

shashank11p Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

kaushal02 Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kaushal02 commented Feb 3, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.