Skip to content

added span to DispatcherServlet.render - #409

Merged
felixbarny merged 10 commits into
masterfrom
unknown repository
Jan 8, 2019
Merged

added span to DispatcherServlet.render#409
felixbarny merged 10 commits into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Jan 7, 2019 •
edited by alvarolobato
Loading

Copy link
Copy Markdown

closes #287

@elasticmachine

elasticmachine commented Jan 7, 2019

Copy link
Copy Markdown

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

felixbarny
felixbarny reviewed Jan 7, 2019
View reviewed changes

@felixbarny felixbarny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Very nice work! The only thing I would change is to add it to the apm-spring-webmvc-plugin module instead of creating a new plugin.

@ghost

ghost commented Jan 7, 2019

Copy link
Copy Markdown
Author

I moved module for existing plugin.

@felixbarny

felixbarny commented Jan 7, 2019

Copy link
Copy Markdown
Member

also, please add an entry to CHANGELOG.md 🙂

@felixbarny

felixbarny commented Jan 7, 2019

Copy link
Copy Markdown
Member

Jenkins test this please

@felixbarny
felixbarny requested a review from eyalkoren January 7, 2019 16:26
@codecov-io

codecov-io commented Jan 7, 2019

Copy link
Copy Markdown

Codecov Report

Merging #409 into master will decrease coverage by 2.19%.
The diff coverage is 41.66%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #409     +/-   ##
===========================================
- Coverage     74.06%   71.86%   -2.2%     
+ Complexity     1433     1269    -164     
===========================================
  Files           145      141      -4     
  Lines          5829     4888    -941     
  Branches        692      496    -196     
===========================================
- Hits           4317     3513    -804     
+ Misses         1283     1152    -131     
+ Partials        229      223      -6
Impacted Files Coverage Δ Complexity Δ
...webmvc/DispatcherServletRenderInstrumentation.java 41.66% <41.66%> (ø) 4 <4> (ø) ⬇️
...ddy/SimpleMethodSignatureOffsetMappingFactory.java 25% <0%> (-75%) 2% <0%> (-1%)
.../agent/plugin/api/AbstractSpanInstrumentation.java 53.19% <0%> (-10.45%) 3% <0%> (-4%)
.../co/elastic/apm/agent/impl/error/ErrorCapture.java 79.62% <0%> (-7.28%) 20% <0%> (-15%)
...pm/agent/report/IntakeV2ReportingEventHandler.java 73.15% <0%> (-5.69%) 30% <0%> (-13%)
...java/co/elastic/apm/agent/bci/ElasticApmAgent.java 72.18% <0%> (-5.55%) 19% <0%> (-10%)
.../apm/agent/report/serialize/DslJsonSerializer.java 81.99% <0%> (-5.4%) 115% <0%> (-31%)
...lastic/apm/agent/impl/transaction/Transaction.java 72% <0%> (-4%) 17% <0%> (-1%)
...va/co/elastic/apm/agent/impl/ElasticApmTracer.java 76.04% <0%> (-2.8%) 44% <0%> (-6%)
...apm/agent/bci/bytebuddy/CustomElementMatchers.java 92% <0%> (-2.74%) 6% <0%> (-3%)
... and 17 more

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 035ba3a...f6b909e. Read the comment docs.

eyalkoren
eyalkoren reviewed Jan 8, 2019
View reviewed changes

@eyalkoren eyalkoren left a comment •
edited
Loading

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.

Awesome!
Not a Spring champion, but looks good to me. Only a small comment about fallback name when view name not available and understanding what the second span is

Comment thread ...src/main/java/co/elastic/apm/agent/spring/webmvc/DispatcherServletRenderInstrumentation.java Outdated
Comment thread ...test/java/co/elastic/apm/agent/spring/webmvc/DispatcherServletRenderInstrumentationTest.java Outdated
felixbarny
felixbarny reviewed Jan 8, 2019
View reviewed changes
Comment thread ...src/main/java/co/elastic/apm/agent/spring/webmvc/DispatcherServletRenderInstrumentation.java Outdated
Comment thread ...src/main/java/co/elastic/apm/agent/spring/webmvc/DispatcherServletRenderInstrumentation.java Outdated
Comment thread ...src/main/java/co/elastic/apm/agent/spring/webmvc/DispatcherServletRenderInstrumentation.java Outdated
Comment thread ...src/main/java/co/elastic/apm/agent/spring/webmvc/DispatcherServletRenderInstrumentation.java Outdated
eyalkoren
eyalkoren approved these changes Jan 8, 2019
View reviewed changes

@eyalkoren eyalkoren 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.

@nugusbayevkk This is awesome, great job! Thanks a lot!

@felixbarny
felixbarny merged commit 95b0572 into elastic:master Jan 8, 2019
@ghost

ghost commented Jan 8, 2019

Copy link
Copy Markdown
Author

thanks all for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add span for DispatcherServlet#render

4 participants

@elasticmachine @felixbarny @codecov-io @eyalkoren