Used BatchSpansProcessor instead of SimpleSpansProcessor #393

RashmiRam · 2020-05-08T06:02:14Z

BatchSpansProcessor won't get blocked by the exporter.
Fixes #368

@trask This is the quick fix. Are we working on introducing the disruptor in the future? If that's the case, Can this just be a stop gap for now?

BatchSpansProcessor won't get blocked by the exporter. Fixes open-telemetry#368 Signed-off-by: RashmiRam <rashmi.ramanathan@freshworks.com>

RashmiRam · 2020-05-08T06:29:43Z

I signed it

johnbley · 2020-05-08T15:18:45Z

Hi Rashmi -
Thanks for the patch! It looks like this is causing the smoke-tests to fail (you can click through the "details" to see the build logs) . You can reproduce this locally by running (e.g.) ./gradlew :smoke-tests:springboot:test. In SpringBootSmokeTest.groovy we use a logging exporter and a simple log reader to check that the spans get logged. Perhaps there's a clean way to explicitly forceFlush the batch or to special-case the logging processor to not be batched?

RashmiRam · 2020-05-08T18:02:36Z

I was trying to run failure test alone as entire instrumentation test takes hours to run. I'll check that. Thanks for the help @johnbley

RashmiRam · 2020-05-08T20:03:49Z

I fixed the springboot smoke tests. Now, I see random failures in instrumentation tests(cassandra, akka-hhtp) which are passing in my local. Any help would be much appreciated.

iNikem · 2020-05-08T20:05:19Z

Yeah, our tests are quite flaky right now. We are working on it :) @trask can you please rerun the pipeline?

iNikem · 2020-05-09T05:43:31Z

@RashmiRam I see that check job has failed in CI with code formatting violation. Can you please run ./gradlew googleJavaFormat as suggested by CONTRIBUTING.md?

RashmiRam · 2020-05-09T06:02:33Z

I did @iNikem and ran ./gradlew auto-tooling:verifyGoogleJavaFormat to find any pending violations. It is passing. I am not sure why the tests in CI are failing

iNikem · 2020-05-09T06:53:42Z

I have checked out your repo and tried to run ./gradlew auto-tooling:verifyGoogleJavaFormat:

The following files are not formatted properly:

/Users/nikem/Documents/workspace/mtp/opentelemetry-auto-instr-java/agent-tooling/src/main/java/io/opentelemetry/auto/tooling/TracerInstaller.java

Did you commit your changes?

RashmiRam · 2020-05-09T07:41:20Z

Yes. I did This is the commit. c4383ba

johnbley · 2020-05-10T13:25:05Z

Yes. I did This is the commit. c4383ba

I've seen googleJavaFormat's cache get confused at times; try adding some whitespace, saving, and then re-running it.

RashmiRam · 2020-05-11T05:45:08Z

That helped @johnbley check job is passing now in CircleCI. Thanks a ton.

johnbley · 2020-05-11T10:13:02Z

smoke-tests/src/main/groovy/io/opentelemetry/smoketest/AbstractSmokeTest.groovy

@@ -70,6 +70,11 @@ abstract class AbstractSmokeTest extends Specification {
    processBuilder.environment().put("JAVA_HOME", System.getProperty("java.home"))
    processBuilder.environment().put("DD_API_KEY", API_KEY)

+//    // Setting configuration variables of batch span processor through env vars
+//    // This config is to immediately flush a batch of 1 span with delay of 10ms
+    processBuilder.environment().put("OTEL_BSP_MAX_EXPORT_BATCH", "1")


I like this solution a lot. Much cleaner than anything I had suggested.

trask · 2020-05-11T19:55:39Z

Hi @RashmiRam, thanks so much for the PR!

It looks like our PlaySmokeTest smoke test is not resilient to the extra delay created by using the BatchSpansProcessor:

Can you try bumping the timeout at PlaySmokeTest.groovy#L48 and SpringBootSmokeTest.groovy#L42 from 10 seconds to 15 seconds?

…nprocessor to finish exporting

RashmiRam · 2020-05-12T09:32:31Z

@trask I have updated the timeout in both play & springbok smoke tests to 20s as 15s fails at times even in my local.

trask

Thanks @RashmiRam!

smoke-tests/play/src/test/groovy/io/opentelemetry/smoketest/PlaySmokeTest.groovy

smoke-tests/springboot/src/test/groovy/io/opentelemetry/smoketest/SpringBootSmokeTest.groovy

…aySmokeTest.groovy

…est/SpringBootSmokeTest.groovy

trask · 2020-05-16T18:24:26Z

Hey @RashmiRam, sorry we haven't merged this yet. The play smoke test is still failing in CI even after I bumped the timeout to 30 seconds. I'm looking into it now.

trask · 2020-05-16T19:18:01Z

The problem is that our smoke tests use the logging exporter and parse the log file which is not reliable under concurrent logging, which can happen now with the background BatchSpanProcessor thread.

Here's an example of the problem, where LOGGED_SPAN didn't end up on it's own line:

[opentelemetry.auto.trace 2020-05-14 22:26:38:428 +0000] [application-akka.actor.default-dispatcher-4] DEBUG io.opentelemetry.auto.bootstrap.instrumentation.java.concurrent.State - Failed to set parent span because another parent span is already set io.opentelemetry.auto.bootstrap.instrumentation.java.concurrent.State@448e2484: new: io.opentelemetry.sdk.trace.RecordEventsReadableSpan@3dc393cf, old: io.opentelemetry.sdk.trace.RecordEventsReadableSpan@3dc393cfLOGGED_SPAN GET /welcome 8c4cd5a61ad2794e

The real solution here is to improve our test harness, I made a note here: #298 (comment)

For now though, the debug logging Failed to set parent span that is causing the interference is unnecessarily verbose, so I fixed it. I had fixed it in #318, but I had reverted my change in #362 because of DataDog/dd-trace-java#1403 (review). I think re-applying #318 is good anyways, and it resolves this issue for now, until we improve the test harness.

RashmiRam · 2020-05-17T04:39:58Z

Thank you so much for the detailed explanation on the test failures @trask and Thanks for the merge too. It would be great if you let me know about the release date for this too.

trask · 2020-05-17T16:54:56Z

Releasing v0.3.0 tomorrow (Mon, May 18) 👍

Used BatchSpansProcessor instead of SimpleSpansProcessor

bb2e299

BatchSpansProcessor won't get blocked by the exporter. Fixes open-telemetry#368 Signed-off-by: RashmiRam <rashmi.ramanathan@freshworks.com>

RashmiRam requested review from jkwatson, safris, trask and tylerbenson as code owners May 8, 2020 06:02

Enable BatchSpanProcessor to use env vars for configuration

7e73a82

iNikem approved these changes May 8, 2020

View reviewed changes

Formatted the code as per the style guidelines

c4383ba

Removed the comment

ff724f4

johnbley reviewed May 11, 2020

View reviewed changes

Updated the timeout for play & spring boot tests to wait for batchspa…

4d04de7

…nprocessor to finish exporting

johnbley approved these changes May 14, 2020

View reviewed changes

trask approved these changes May 14, 2020

View reviewed changes

Merge branch 'master' into master

14bf4bd

trask reviewed May 14, 2020

View reviewed changes

smoke-tests/play/src/test/groovy/io/opentelemetry/smoketest/PlaySmokeTest.groovy Outdated Show resolved Hide resolved

smoke-tests/springboot/src/test/groovy/io/opentelemetry/smoketest/SpringBootSmokeTest.groovy Outdated Show resolved Hide resolved

trask added 2 commits May 14, 2020 13:58

Update smoke-tests/play/src/test/groovy/io/opentelemetry/smoketest/Pl…

4b3b64b

…aySmokeTest.groovy

Update smoke-tests/springboot/src/test/groovy/io/opentelemetry/smoket…

d40a649

…est/SpringBootSmokeTest.groovy

Merge branch 'master' into master

e38fba2

trask added 3 commits May 16, 2020 11:55

Remove unnecessarily verbose debug logging

499838b

Merge branch 'master' into master

1817347

Revert timeout change

df97801

trask merged commit 29a18bd into open-telemetry:master May 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Used BatchSpansProcessor instead of SimpleSpansProcessor #393

Used BatchSpansProcessor instead of SimpleSpansProcessor #393

RashmiRam commented May 8, 2020

RashmiRam commented May 8, 2020

johnbley commented May 8, 2020 •

edited

Loading

RashmiRam commented May 8, 2020

RashmiRam commented May 8, 2020

iNikem commented May 8, 2020

iNikem commented May 9, 2020

RashmiRam commented May 9, 2020 •

edited

Loading

iNikem commented May 9, 2020

RashmiRam commented May 9, 2020

johnbley commented May 10, 2020

RashmiRam commented May 11, 2020

johnbley May 11, 2020

trask commented May 11, 2020

RashmiRam commented May 12, 2020

trask left a comment

trask commented May 16, 2020

trask commented May 16, 2020

RashmiRam commented May 17, 2020 •

edited

Loading

trask commented May 17, 2020

Used BatchSpansProcessor instead of SimpleSpansProcessor #393

Used BatchSpansProcessor instead of SimpleSpansProcessor #393

Conversation

RashmiRam commented May 8, 2020

RashmiRam commented May 8, 2020

johnbley commented May 8, 2020 • edited Loading

RashmiRam commented May 8, 2020

RashmiRam commented May 8, 2020

iNikem commented May 8, 2020

iNikem commented May 9, 2020

RashmiRam commented May 9, 2020 • edited Loading

iNikem commented May 9, 2020

RashmiRam commented May 9, 2020

johnbley commented May 10, 2020

RashmiRam commented May 11, 2020

johnbley May 11, 2020

Choose a reason for hiding this comment

trask commented May 11, 2020

RashmiRam commented May 12, 2020

trask left a comment

Choose a reason for hiding this comment

trask commented May 16, 2020

trask commented May 16, 2020

RashmiRam commented May 17, 2020 • edited Loading

trask commented May 17, 2020

johnbley commented May 8, 2020 •

edited

Loading

RashmiRam commented May 9, 2020 •

edited

Loading

RashmiRam commented May 17, 2020 •

edited

Loading