Skip to content
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

added span to DispatcherServlet.render #409

Merged
merged 10 commits into from Jan 8, 2019
Merged

added span to DispatcherServlet.render #409

merged 10 commits into from Jan 8, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 7, 2019

closes #287

@elasticmachine
Copy link

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?

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.

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
Copy link
Author

ghost commented Jan 7, 2019

I moved module for existing plugin.

@felixbarny
Copy link
Member

also, please add an entry to CHANGELOG.md 🙂

@felixbarny
Copy link
Member

Jenkins test this please

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

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.

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.

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

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.

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

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

ghost commented Jan 8, 2019

thanks all for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add span for DispatcherServlet#render
4 participants