add support for opencensus http servlet plugin in spring cloud framework #1650

rghetia · 2019-01-04T00:15:44Z

No description provided.

rghetia · 2019-01-04T00:50:49Z

check the build failure.

codecov-io · 2019-01-04T05:18:30Z

Codecov Report

❗ No coverage uploaded for pull request base (master@8312f0e). Click here to learn what that means.
The diff coverage is 14.28%.

@@           Coverage Diff            @@
##             master   #1650   +/-   ##
========================================
  Coverage          ?   82.3%           
  Complexity        ?    1987           
========================================
  Files             ?     294           
  Lines             ?    9179           
  Branches          ?     884           
========================================
  Hits              ?    7555           
  Misses            ?    1336           
  Partials          ?     288

Impacted Files	Coverage Δ	Complexity Δ
...spring/autoconfig/OpenCensusAutoConfiguration.java	`0% <0%> (ø)`	`0 <0> (?)`
...ontrib/spring/autoconfig/OpenCensusProperties.java	`0% <0%> (ø)`	`0 <0> (?)`
...ntrib/spring/instrument/web/HttpServletFilter.java	`100% <100%> (ø)`	`1 <1> (?)`

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 8312f0e...e9cc809. Read the comment docs.

rghetia · 2019-01-06T21:45:49Z

...java/io/opencensus/contrib/springcloud/instrument/web/HttpServletFilterIntegrationTests.java

+  }
+
+  @Test(timeout = 10000)
+  public void shouldCreateAServerTrace() throws Exception {


rename it to shouldCreateServerTrace

rghetia · 2019-01-06T21:47:50Z

...d/core/src/main/java/io/opencensus/contrib/springcloud/instrument/web/HttpServletFilter.java

+import org.springframework.stereotype.Component;
+
+@Component
+@Order(1)


Change to order to lower value perhaps org.springframework.core.Ordered.HIGHEST_PRECEDENCE + 10

@rghetia if you are interested in timing information about the HttpRequest then Ordered.Highest_Precedence+10 would give incorrect information. Your filter should be the first filter in the filter chain.

This is also necessary should opencensus in future decide to auto collect the exceptions because 500s usually get caught at the very last Spring filter in the Filter chain. .

I used +10 to leave some room for other important filter (may be security related) to precede tracing/monitoring. +10 may be too much. do you have a recommendation here?

@rghetia we had similar problem in Application insights and there was a discussion around it.

You may want to refer microsoft/ApplicationInsights-Java#766 (comment) for more relevant details.

in short there are some default springboot filters which are responsible for setting error codes, and get the correct timing. It would be very hard to get that details if the filter lies below that in the chain. Therefore HIGHEST_PRECEDENCE is needed imo. But, I am open if you find better alternatives

I would recommend the exact same value that Springs Tracing uses for their filter, unless you expect them both to be there, then maybe an additional + 1

static final int TRACING_FILTER_ORDER = Ordered.HIGHEST_PRECEDENCE + 5;

SG. I'll make the change.

@devinsba yes your suggestion looks more apt. @rghetia I would recommend this change.

meltsufin · 2019-01-09T15:56:30Z

...d/core/src/main/java/io/opencensus/contrib/springcloud/instrument/web/HttpServletFilter.java

+@Component
+@Order(OpenCensusAutoConfiguration.TRACE_FILTER_ORDER)
+@SuppressFBWarnings("RI_REDUNDANT_INTERFACES")
+public class HttpServletFilter extends OcHttpServletFilter implements Filter {}


An alternative to using the Servet filter would be to use a Spring MVC interceptor and the WebMvcConfigurer. We did this for including the trace id in the logs. The same concept applies to this PR. Take a look in our code here.

thanks for the review. Is Servlet filter approach still ok? The reason I asked is the most of the work for servlet filter is done and usable outside of the spring frame work as well. But if there are advantages of doing it using MVC interceptor i am can certainly change it.

Sure, Servlet Filter is more generic.

...ud/core/src/main/java/io/opencensus/contrib/springcloud/autoconfig/OpenCensusProperties.java

bogdandrutu · 2019-01-23T20:42:06Z

all/build.gradle

@@ -19,6 +19,7 @@ def subprojects = [
        project(':opencensus-contrib-log-correlation-stackdriver'),
        project(':opencensus-contrib-monitored-resource-util'),
        project(':opencensus-contrib-spring'),
+        project(':opencensus-contrib-spring-cloud-core'),


Can this be part of the opencensus-contrib-spring?

bogdandrutu · 2019-01-23T20:43:12Z

.../src/main/java/io/opencensus/contrib/springcloud/autoconfig/OpenCensusAutoConfiguration.java

+@EnableConfigurationProperties(OpenCensusProperties.class)
+@ExperimentalApi
+public class OpenCensusAutoConfiguration {
+  public static final int TRACE_FILTER_ORDER = Ordered.HIGHEST_PRECEDENCE + 10;


Comment about what this field represents?

It represents the order in which HttpServletFilter will be invoked. I have added comments as to what it is in the new update.

rghetia

@dinooliva and @bogdandrutu PTAL.

rghetia · 2019-02-14T00:22:06Z

.../src/main/java/io/opencensus/contrib/springcloud/autoconfig/OpenCensusAutoConfiguration.java

+@EnableConfigurationProperties(OpenCensusProperties.class)
+@ExperimentalApi
+public class OpenCensusAutoConfiguration {
+  public static final int TRACE_FILTER_ORDER = Ordered.HIGHEST_PRECEDENCE + 10;


It represents the order in which HttpServletFilter will be invoked. I have added comments as to what it is in the new update.

devinsba · 2019-02-23T02:03:43Z

...pring/src/main/java/io/opencensus/contrib/spring/autoconfig/OpenCensusAutoConfiguration.java

+ */
+@Configuration
+@ComponentScan(basePackages = "io.opencensus")
+@ConditionalOnProperty(value = "spring.opencensus.enabled", matchIfMissing = true)


You should be careful to use the same property namespaces as spring projects. The spring teams did a bunch of work to consolidate their use of properties to mostly be under spring. so that the community could have other areas. I would suggest you make opencensus the top level namespace

I'll change the name.

bogdandrutu · 2019-05-10T23:41:01Z

What do we need to ship this?

rghetia · 2019-05-13T17:00:42Z

What do we need to ship this?

Need to add ability to specify propagator, extractor and publicEndpoint.
Also needs ability to enable just one tracing or metrics but not both. In the current implementation it does both. This can probably wait.

dinooliva · 2019-05-13T20:41:56Z

As discussed irl, agree that we don't need to separate controls for tracing and metrics.

rghetia · 2019-05-16T17:01:52Z

@dinooliva and @bogdandrutu PTAL.

googlebot added the cla: yes label Jan 4, 2019

rghetia requested review from bogdandrutu and dinooliva January 5, 2019 16:28

rghetia commented Jan 7, 2019

View reviewed changes

meltsufin reviewed Jan 9, 2019

View reviewed changes

dinooliva reviewed Jan 22, 2019

View reviewed changes

...ud/core/src/main/java/io/opencensus/contrib/springcloud/autoconfig/OpenCensusProperties.java Outdated Show resolved Hide resolved

bogdandrutu reviewed Jan 23, 2019

View reviewed changes

rghetia force-pushed the spring_servlet branch from 5af99c5 to 37de0e7 Compare February 13, 2019 22:30

rghetia commented Feb 22, 2019

View reviewed changes

devinsba reviewed Feb 23, 2019

View reviewed changes

rghetia added 7 commits May 15, 2019 16:43

add support for opencensus http servlet plugin in spring cloud framework

711fc5d

fix build error.

b57f6dd

add spring Test Version.

b6417bb

Changed filter order.

bcedc7a

move to existing spring artifact.

812bf4a

fix review comment. remove files from spring-core.

a1204a1

fix @SInCE annotations.

e9cc809

rghetia force-pushed the spring_servlet branch from 37de0e7 to e9cc809 Compare May 16, 2019 05:57

rghetia requested review from songy23 and a team as code owners May 16, 2019 05:57

dinooliva approved these changes May 17, 2019

View reviewed changes

rghetia merged commit a95cc72 into census-instrumentation:master May 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for opencensus http servlet plugin in spring cloud framework #1650

add support for opencensus http servlet plugin in spring cloud framework #1650

rghetia commented Jan 4, 2019

rghetia commented Jan 4, 2019

codecov-io commented Jan 4, 2019 •

edited

rghetia Jan 6, 2019

rghetia Jan 6, 2019

dhaval24 Jan 8, 2019 •

edited

rghetia Jan 8, 2019

dhaval24 Jan 13, 2019

devinsba Jan 21, 2019

rghetia Jan 22, 2019

dhaval24 Jan 25, 2019 •

edited

meltsufin Jan 9, 2019

rghetia Jan 10, 2019

meltsufin Jan 10, 2019

bogdandrutu Jan 23, 2019

bogdandrutu Jan 23, 2019

rghetia Feb 14, 2019

rghetia left a comment

rghetia Feb 14, 2019

devinsba Feb 23, 2019

rghetia Feb 25, 2019

bogdandrutu commented May 10, 2019

rghetia commented May 13, 2019

dinooliva commented May 13, 2019

rghetia commented May 16, 2019

add support for opencensus http servlet plugin in spring cloud framework #1650

add support for opencensus http servlet plugin in spring cloud framework #1650

Conversation

rghetia commented Jan 4, 2019

rghetia commented Jan 4, 2019

codecov-io commented Jan 4, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaval24 Jan 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhaval24 Jan 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rghetia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented May 10, 2019

rghetia commented May 13, 2019

dinooliva commented May 13, 2019

rghetia commented May 16, 2019

codecov-io commented Jan 4, 2019 •

edited

dhaval24 Jan 8, 2019 •

edited

dhaval24 Jan 25, 2019 •

edited