feat(servlet): content length #726

FrankSpitulski · 2020-07-18T00:41:26Z

add content length tag for v3 servlets as per https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#common-attributes

linux-foundation-easycla · 2020-07-18T00:41:29Z

❌The commit (b2ee742) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve. For further assistance with EasyCLA, please submit a support request ticket.

anuraaga

Thanks for helping with this!

anuraaga · 2020-07-18T02:34:54Z

agent-bootstrap/src/main/java/io/opentelemetry/auto/instrumentation/api/MoreAttributes.java

@@ -19,6 +19,7 @@
 public class MoreAttributes {
  public static final String HTTP_QUERY = "http.query.string";
  public static final String HTTP_FRAGMENT = "http.fragment.string";
+  public static final String HTTP_RESPONSE_CONTENT_LENGTH = "http.response_content_length";


If I recall correctly this was already released in otel 0.6.0, is it not part of SemanticAttributes?

I did a project wide search for the string before adding it. Is that defined elsewhere?

Yeah it's in the SDK repo

https://github.com/open-telemetry/opentelemetry-java/blob/master/api/src/main/java/io/opentelemetry/trace/attributes/SemanticAttributes.java

anuraaga · 2020-07-18T02:40:42Z

...ain/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/CountingHttpServletResponse.java

+import javax.servlet.http.HttpServletResponseWrapper;
+
+/** HttpServletResponseWrapper since servlet 2.3, not applicable to 2.2 */
+public class CountingHttpServletResponse extends HttpServletResponseWrapper {


I think we may need counting for cases like chunked responses but a huge majority will have a content length header. What do you think about starting with that to get most bang for buck? We could do it in HttpServerTracer I think.

Either way we probably want to avoid adding this wrapper when the response has a content length header. We could use the content length in a future PR instead.

From my testing the set Content Length method is not invoked by springboot's rest controller. Counting the bytes was the only reliable way to get the size. I also tried to stick it in the http server tracer but the wrapper was introduced in servlet 2.3 and causes issues in the servlet 2.2 instrumentation.

I see that sounds like for servlet we'll need to ignore the content length header always, thanks for the background

trask

Thanks @FrankSpitulski!

Do we also need to wrap/count for HttpServletResponse.getWriter()?

Check out the CI failures when you have time.

:instrumentation:dropwizard-testing:test is failing because it isn't expecting the new attribute.

There's also a muzzle build failure for :instrumentation:jetty-8.0:muzzle.

To fix the muzzle issues, add:

      "io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletResponse",
      "io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletResponse$CountingServletOutputStream",

to JettyHandlerInstrumentation.helperClassNames() (same as you already did in Servlet3Instrumentation).

Don't hesitate to reach out if muzzle gives you any more issues. It's mysterious and undocumented currently (#536).

trask · 2020-07-18T18:54:55Z

...ain/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/CountingHttpServletResponse.java

+    @Override
+    public void write(byte[] b, int off, int len) throws IOException {
+      delegate.write(b, off, len);
+      counter += len - off;


Suggested change

counter += len - off;

counter += len;

FrankSpitulski · 2020-07-18T19:59:56Z

For completeness we should wrap getWriter, but in practice I don't think it is used on the httpservletresponse. The httpservletresponse overrides the outputstream and the use of the writer vs the output stream is mutually exclusive according to the docs. I can add a wrapper for it but I don't have a real world test case that uses it to verify.

trask · 2020-07-19T18:50:24Z

I don't have a real world test case that uses it to verify.

I think the jsp-2.3 module tests (which also test the servlet-3.0 instrumentation) should use getWriter(), which would give us test coverage

.../servlet/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java

iNikem · 2020-07-20T06:02:56Z

...ain/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/AsyncContextInstrumentation.java

@@ -50,6 +50,9 @@ public AsyncContextInstrumentation() {
    return new String[] {
      "io.opentelemetry.instrumentation.servlet.HttpServletRequestGetter",
      "io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer",
+      "io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletResponse",


Use packageName as below

iNikem · 2020-07-20T06:03:53Z

...rc/main/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/Servlet3Instrumentation.java

@@ -54,6 +54,9 @@ public Servlet3Instrumentation() {
    return new String[] {
      "io.opentelemetry.instrumentation.servlet.HttpServletRequestGetter",
      "io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer",
+      "io.opentelemetry.auto.instrumentation.servlet.v3_0.CountingHttpServletResponse",


Use packageName as below

trask · 2020-07-20T22:49:17Z

instrumentation/servlet/servlet-3.0/src/test/groovy/AbstractServlet3Test.groovy

@@ -88,6 +88,8 @@ abstract class AbstractServlet3Test<SERVER, CONTEXT> extends HttpServerTest<SERV
        "${SemanticAttributes.HTTP_URL.key()}" { it == "${endpoint.resolve(address)}" || it == "${endpoint.resolveWithoutFragment(address)}" }
        "${SemanticAttributes.HTTP_METHOD.key()}" method
        "${SemanticAttributes.HTTP_STATUS_CODE.key()}" endpoint.status
+        "${SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH.key()}" { it == null || it == getContentLength(endpoint) }


curious under what conditions is response content length not captured? (why we need it == null)

@trask There are quite a few where response length is not captured in the tests.

I haven't figured out how to track down why that is happening yet. The groovy tests are a lot less straightforward than junit.

All of the sync tests are happy, maybe my async implementation is bad.

checking the actual okhttp response body length in the test fixed the async tests. there is only one special case that ignores the content length, exceptions. Those don't hit the httpservletresponse as far as I can tell so I'm not sure where to count them.

add content length tag for v3 servlets as per https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#common-attributes

FrankSpitulski · 2020-07-21T03:33:26Z

gradlew.bat

@@ -1,104 +1,104 @@
-@rem


I cannot get git to leave this file alone. It changes it to have CRLF but according to the .gitattributes that should already be the case.

yeah, we just changed the .gitattributes file.

can you try:

rm .gitattributes git checkout -- .gitattributes

oh i just got stuck in this again now too, will debug, really not understanding this either

this seems to make the problem go away at least temporarily:

sed -i '/.bat/d' .gitattributes && git status > /dev/null && git checkout -- .gitattributes

i think i found permanent fix #743, sorry for the confusion 😅

FrankSpitulski · 2020-07-21T05:05:11Z

all tests good now

FrankSpitulski · 2020-07-21T05:50:28Z

I think the ci is having some interment failures. I'll squash a commit to trigger it again.

trask · 2020-07-21T05:56:53Z

I think the ci is having some interment failures. I'll squash a commit to trigger it again.

Yeah, we have several sporadic failures currently: https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues?q=is%3Aissue+is%3Aopen+label%3A%22sporadic+test+failure%22

We'll watch the build and hit retry if it fails again.

...ain/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/CountingHttpServletResponse.java

trask

Thanks @FrankSpitulski, this is a great addition!

A couple of suggestions/comments below.

...ain/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/CountingHttpServletResponse.java

testing-common/src/main/groovy/io/opentelemetry/auto/test/base/HttpServerTest.groovy

...ain/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/CountingHttpServletResponse.java

…etry/auto/instrumentation/servlet/v3_0/CountingHttpServletResponse.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

refactor tests

trask

👍

FrankSpitulski · 2020-07-21T23:23:08Z

@trask not sure what's going on with that failed test

groovy.lang.MissingMethodException: No signature of method: FinatraServer.started() is applicable for argument types: () values: []
Possible solutions: start(), stage(), trace(scala.Function0), trace(org.slf4j.Marker, scala.Function0), trace(scala.Function0, java.lang.Throwable), trace(org.slf4j.Marker, scala.Function0, java.lang.Throwable)
	at FinatraServerTest.startServer(FinatraServerTest.groovy:57)

started is an ~~implicit~~ injected function from the twitter scala inject-app library. The IDE seems to find it fine and I don't think I changed anything that would cause it to fail.

trask · 2020-07-21T23:28:16Z

yeah, the test_latest build runs the tests against latest dependencies (in this case the latest finatra dependency) available in maven central, so sometimes it fails when there's a new version released, and i can see there was a new finatra version released just today

https://search.maven.org/search?q=g:com.twitter%20a:finatra-http_2.11

so that maybe the issue, i'll check it out and get back to you

trask · 2020-07-21T23:41:02Z

Yeah, that's the problem. I submitted #747 to fix. And then I think I can merge master into your PR to get clean build, or if not I will ping you to merge master into your branch. thx for your patience as we go through growing pains 😅

trask · 2020-07-22T03:25:10Z

Thanks again @FrankSpitulski 🎉

FrankSpitulski requested review from anuraaga, iNikem, jkwatson, trask and tylerbenson as code owners July 18, 2020 00:41

FrankSpitulski mentioned this pull request Jul 18, 2020

Adding servlet http.response_content_length as per spec #727

Closed

FrankSpitulski force-pushed the feat/servlet/content-length branch from b2ee742 to ff22b26 Compare July 18, 2020 01:01

anuraaga reviewed Jul 18, 2020

View reviewed changes

trask reviewed Jul 18, 2020

View reviewed changes

iNikem reviewed Jul 19, 2020

View reviewed changes

.../servlet/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java Show resolved Hide resolved

trask mentioned this pull request Jul 20, 2020

How best to capture response attributes in HttpServerTracer #733

Closed

iNikem reviewed Jul 20, 2020

View reviewed changes

FrankSpitulski force-pushed the feat/servlet/content-length branch from 24abd5a to 6476633 Compare July 20, 2020 19:47

trask reviewed Jul 20, 2020

View reviewed changes

FrankSpitulski added 4 commits July 20, 2020 20:28

feat(servlet): content length

920bd18

add content length tag for v3 servlets as per https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#common-attributes

address bugs in PR

0a62408

count printwriter and add null checks

8f4c061

add tests

0f6a6ab

FrankSpitulski force-pushed the feat/servlet/content-length branch from e13a34f to 7e0f082 Compare July 21, 2020 03:29

FrankSpitulski commented Jul 21, 2020

View reviewed changes

trask mentioned this pull request Jul 21, 2020

Turn gradlew.bat into a text file #743

Merged

FrankSpitulski force-pushed the feat/servlet/content-length branch from 8b6035b to c7eedc4 Compare July 21, 2020 04:47

fix tests

ec0f05a

FrankSpitulski force-pushed the feat/servlet/content-length branch from 6e5c14b to ec0f05a Compare July 21, 2020 05:53

anuraaga approved these changes Jul 21, 2020

View reviewed changes

...ain/java/io/opentelemetry/auto/instrumentation/servlet/v3_0/CountingHttpServletResponse.java Outdated Show resolved Hide resolved

trask reviewed Jul 21, 2020

View reviewed changes

FrankSpitulski and others added 2 commits July 21, 2020 14:36

Update instrumentation/servlet/servlet-3.0/src/main/java/io/opentelem…

7df2596

…etry/auto/instrumentation/servlet/v3_0/CountingHttpServletResponse.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

Update instrumentation/servlet/servlet-3.0/src/main/java/io/opentelem…

c7884a6

…etry/auto/instrumentation/servlet/v3_0/CountingHttpServletResponse.java Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>

FrankSpitulski force-pushed the feat/servlet/content-length branch from e780889 to f799b7f Compare July 21, 2020 22:25

remove atomic print writer counter

ad25899

refactor tests

FrankSpitulski force-pushed the feat/servlet/content-length branch from f799b7f to ad25899 Compare July 21, 2020 22:56

trask approved these changes Jul 21, 2020

View reviewed changes

trask mentioned this pull request Jul 21, 2020

Temporary fix for finatra latestDepTest failure #747

Merged

trask mentioned this pull request Jul 22, 2020

Don't run latestDepTest on PRs #748

Closed

Merge remote-tracking branch 'upstream/master' into frankspitulski

d440578

trask merged commit d6e39f8 into open-telemetry:master Jul 22, 2020

FrankSpitulski deleted the feat/servlet/content-length branch July 29, 2020 17:56

iNikem mentioned this pull request Oct 5, 2020

Capture request and response bodies #1317

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(servlet): content length #726

feat(servlet): content length #726

FrankSpitulski commented Jul 18, 2020

linux-foundation-easycla bot commented Jul 18, 2020

anuraaga left a comment

anuraaga Jul 18, 2020

FrankSpitulski Jul 18, 2020

anuraaga Jul 18, 2020

anuraaga Jul 18, 2020

FrankSpitulski Jul 18, 2020

anuraaga Jul 18, 2020

trask left a comment

trask Jul 18, 2020

FrankSpitulski Jul 18, 2020

FrankSpitulski commented Jul 18, 2020

trask commented Jul 19, 2020

iNikem Jul 20, 2020

iNikem Jul 20, 2020

trask Jul 20, 2020

FrankSpitulski Jul 20, 2020

FrankSpitulski Jul 20, 2020

FrankSpitulski Jul 21, 2020 •

edited

Loading

FrankSpitulski Jul 21, 2020

trask Jul 21, 2020

trask Jul 21, 2020

trask Jul 21, 2020 •

edited

Loading

trask Jul 21, 2020

FrankSpitulski Jul 21, 2020

FrankSpitulski commented Jul 21, 2020

FrankSpitulski commented Jul 21, 2020

trask commented Jul 21, 2020

trask left a comment

trask left a comment

FrankSpitulski commented Jul 21, 2020 •

edited

Loading

trask commented Jul 21, 2020

trask commented Jul 21, 2020

trask commented Jul 22, 2020

feat(servlet): content length #726

feat(servlet): content length #726

Conversation

FrankSpitulski commented Jul 18, 2020

linux-foundation-easycla bot commented Jul 18, 2020

anuraaga 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankSpitulski commented Jul 18, 2020

trask commented Jul 19, 2020

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

FrankSpitulski Jul 21, 2020 • edited Loading

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

trask Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankSpitulski commented Jul 21, 2020

FrankSpitulski commented Jul 21, 2020

trask commented Jul 21, 2020

trask left a comment

Choose a reason for hiding this comment

trask left a comment

Choose a reason for hiding this comment

FrankSpitulski commented Jul 21, 2020 • edited Loading

trask commented Jul 21, 2020

trask commented Jul 21, 2020

trask commented Jul 22, 2020

FrankSpitulski Jul 21, 2020 •

edited

Loading

trask Jul 21, 2020 •

edited

Loading

FrankSpitulski commented Jul 21, 2020 •

edited

Loading