Skip to content

Instrument View#render instead of DispatcherServlet#render #829

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 28 commits into from
Sep 27, 2019
Merged

Instrument View#render instead of DispatcherServlet#render #829

merged 28 commits into from
Sep 27, 2019

Conversation

@videnkz
Copy link
Contributor

@videnkz videnkz commented Sep 3, 2019 •
edited by eyalkoren
Loading

Checklist

closes #811

  • Implement code
  • Add tests
    -- [x] jsp
    -- [x] markup templates
    -- [x] freemarker
    -- [x] groovy templates
    -- [x] jackson2json view
    -- [x] jade4j template
  • Update documentation
  • Update CHANGELOG.md
  • Update supported-technologies.asciidoc
  • Added an API method or config option? Document in which version this will be introduced.
@videnkz videnkz changed the title Issue 811 template for render WIP: Issue 811 template for render Sep 3, 2019
@videnkz videnkz changed the title WIP: Issue 811 template for render Issue 811 instrument view render Sep 3, 2019
@videnkz
Copy link
Contributor Author

videnkz commented Sep 3, 2019

Hi @felixbarny ,
please review pull request.
@videnkz
Copy link
Contributor Author

videnkz commented Sep 4, 2019

image
image
@codecov-io
Copy link

codecov-io commented Sep 5, 2019 •
edited
Loading

Codecov Report

Merging #829 into master will decrease coverage by 0.16%.
The diff coverage is 0%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #829 +/- ## ============================================ - Coverage 64.3% 64.13% -0.17%  Complexity 84 84 ============================================ Files 232 231 -1 Lines 9592 9353 -239 Branches 1275 1215 -60 ============================================ - Hits 6168 5999 -169  + Misses 3047 2973 -74  - Partials 377 381 +4
Impacted Files Coverage Δ Complexity Δ
...co/elastic/apm/agent/servlet/ServletApiAdvice.java 7.5% <0%> (-0.1%) 0 <0> (ø)
.../apm/agent/report/serialize/DslJsonSerializer.java 86.08% <0%> (-3.63%) 0% <0%> (ø)
...m/agent/jms/JmsMessageListenerInstrumentation.java 23.8% <0%> (-1.2%) 0% <0%> (ø)
...lastic/apm/agent/jms/JmsInstrumentationHelper.java 100% <0%> (ø) 0% <0%> (ø) ⬇️
.../elastic/apm/agent/jms/BaseJmsInstrumentation.java 100% <0%> (ø) 0% <0%> (ø) ⬇️
...ic/apm/agent/jms/JmsInstrumentationHelperImpl.java 0% <0%> (ø) 0% <0%> (ø) ⬇️
.../elastic/apm/agent/jms/MessagingConfiguration.java
...m/agent/jms/JmsMessageConsumerInstrumentation.java 26.56% <0%> (+0.75%) 0% <0%> (ø) ⬇️
...agent/spring/webmvc/ViewRenderInstrumentation.java 50% <0%> (+11.9%) 0% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83c6981...64207f0. Read the comment docs.
felixbarny
felixbarny requested changes Sep 19, 2019
View reviewed changes
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

Awesome job! Sorry for the slow review.
eyalkoren
eyalkoren reviewed Sep 22, 2019
View reviewed changes
Copy link
Contributor

@eyalkoren eyalkoren left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Well done, thanks!!

Two small suggestions.

Also, please merge and resolve conflicts so that we can run the tests.
eyalkoren
eyalkoren reviewed Sep 26, 2019
View reviewed changes
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Great, thanks!
A few minor comments.
In addition, since the default of obtainSubtype is not tested, please add a small sanity test for this method only, eg obtainSubtype(MyCustomView) yielding MyCustom.
@felixbarny felixbarny changed the title Issue 811 instrument view render Instrument View#render instead of DispatcherServlet#render Sep 27, 2019
@felixbarny
Copy link
Member

felixbarny commented Sep 27, 2019

I've added the suggestions, hope you don't mind 🙂
Also found and fixed some edge cases which could lead to a StringIndexOutOfBoundsException for really strange class names and for those not containing "View".
@videnkz
Copy link
Contributor Author

videnkz commented Sep 27, 2019

I've added the suggestions, hope you don't mind slightly_smiling_face
Also found and fixed some edge cases which could lead to a StringIndexOutOfBoundsException for really strange class names and for those not containing "View".

Thanks :)
I also thought about classes without packages
@felixbarny felixbarny merged commit 246b7d3 into elastic:master Sep 27, 2019
@felixbarny
Copy link
Member

felixbarny commented Sep 27, 2019

Thanks for bearing with us even if it sometimes takes some time to get changes in, but we really appreciate it!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants

@videnkz @codecov-io @felixbarny @eyalkoren @lreuven @knugusbayevps