Rxjava 2 #2130

piotr-sumo · 2021-01-27T15:45:00Z

No description provided.

linux-foundation-easycla · 2021-01-27T15:45:05Z

The committers are authorized under a signed CLA.

✅ Piotr Glazar (77bc145, 356a611)

piotr-sumo · 2021-01-28T10:24:36Z

Dear reviewers, I need some help with this build. It fails while locally I can assemble the project.
For example, ./gradlew :instrumentation:rabbitmq-2.7:javaagent:test fails locally with com.github.dockerjava.api.exception.NotFoundException: Status 404: {"message":"No such image: testcontainers/ryuk:0.3.0"} which is not related to my changes.
The same is with :instrumentation:apache-camel-2.20:javaagent:test.

jkwatson · 2021-01-29T16:20:50Z

@iNikem @trask @anuraaga any ideas on helping @piotr-sumo here?

iNikem · 2021-01-29T18:33:24Z

@piotr-sumo Can you run tests in the main branch of this repo? I mean, is this problem of this PR or your environment in general?

piotr-sumo · 2021-02-01T08:07:05Z

@iNikem I will run tests on main and report update here.

piotr-sumo · 2021-02-01T09:12:57Z

@iNikem I've run tests from fresh master on jdk 15 and it fails unfortunately.

    java.rmi.ConnectException: Connection refused to host: 52.6.177.231; nested exception is:
        java.net.ConnectException: Operation timed out

Do you have any ideas how to fix it?

anuraaga · 2021-02-01T09:18:48Z

@piotr-sumo Do you mind pasting the larger stack trace there?

piotr-sumo · 2021-02-01T09:24:53Z

@anuraaga this is one of the failing tests:

> Task :instrumentation:rmi:javaagent:test

RmiTest > Client call creates spans FAILED
    java.rmi.ConnectException: Connection refused to host: 54.159.152.5; nested exception is:
        java.net.ConnectException: Operation timed out
        at java.rmi/sun.rmi.transport.tcp.TCPEndpoint.newSocket(TCPEndpoint.java:623)
        at java.rmi/sun.rmi.transport.tcp.TCPChannel.createConnection(TCPChannel.java:209)
        at java.rmi/sun.rmi.transport.tcp.TCPChannel.newConnection(TCPChannel.java:196)
        at java.rmi/sun.rmi.server.UnicastRef.invoke(UnicastRef.java:132)
        at java.rmi/java.rmi.server.RemoteObjectInvocationHandler.invokeRemoteMethod(RemoteObjectInvocationHandler.java:217)
        at java.rmi/java.rmi.server.RemoteObjectInvocationHandler.invoke(RemoteObjectInvocationHandler.java:162)
        at RmiTest.Client call creates spans_closure1(RmiTest.groovy:37)
        at RmiTest.Client call creates spans_closure1(RmiTest.groovy)
        at io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace_closure1(TraceUtils.groovy:32)
        at groovy.lang.Closure.call(Closure.java:405)
        at groovy.lang.Closure.call(Closure.java:421)
        at io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace(TraceUtils.groovy:31)
        at RmiTest.Client call creates spans(RmiTest.groovy:35)

        Caused by:
        java.net.ConnectException: Operation timed out
            at java.base/sun.nio.ch.Net.connect(Net.java:574)
            at java.base/sun.nio.ch.Net.connect(Net.java:563)
            at java.base/sun.nio.ch.NioSocketImpl.connect(NioSocketImpl.java:588)
            at java.base/java.net.SocksSocketImpl.connect(SocksSocketImpl.java:333)
            at java.base/java.net.Socket.connect(Socket.java:648)
            at java.base/java.net.Socket.connect(Socket.java:597)
            at java.base/java.net.Socket.<init>(Socket.java:520)
            at java.base/java.net.Socket.<init>(Socket.java:294)
            at java.rmi/sun.rmi.transport.tcp.TCPDirectSocketFactory.createSocket(TCPDirectSocketFactory.java:40)
            at java.rmi/sun.rmi.transport.tcp.TCPEndpoint.newSocket(TCPEndpoint.java:617)
            ... 12 more

mateuszrzeszutek · 2021-02-01T10:38:07Z

@piotr-sumo Are you using any VPN or proxy server?

piotr-sumo · 2021-02-01T10:40:41Z

@mateuszrzeszutek Yes, I have corporate VPN turned on. Let me run those tests with VPN disabled.

piotr-sumo · 2021-02-01T10:47:12Z

@mateuszrzeszutek withouth VPN I get


SpymemcachedTest > initializationError FAILED
    org.testcontainers.containers.ContainerLaunchException: Container startup failed
        at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:331)
        at org.testcontainers.containers.GenericContainer.start(GenericContainer.java:312)
        at SpymemcachedTest.setupSpec(SpymemcachedTest.groovy:51)

        Caused by:
        org.testcontainers.containers.ContainerFetchException: Can't get Docker image: RemoteDockerImage(imageName=memcached:latest, imagePullPolicy=DefaultPullPolicy())
            at org.testcontainers.containers.GenericContainer.getDockerImageName(GenericContainer.java:1282)
            at org.testcontainers.containers.GenericContainer.logger(GenericContainer.java:616)
            at org.testcontainers.containers.GenericContainer.doStart(GenericContainer.java:321)
            ... 2 more

            Caused by:
            com.github.dockerjava.api.exception.NotFoundException: Status 404: {"message":"No such image: testcontainers/ryuk:0.3.0"}
                at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.execute(DefaultInvocationBuilder.java:241)
                at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.post(DefaultInvocationBuilder.java:125)
                at org.testcontainers.shaded.com.github.dockerjava.core.exec.CreateContainerCmdExec.execute(CreateContainerCmdExec.java:33)
                at org.testcontainers.shaded.com.github.dockerjava.core.exec.CreateContainerCmdExec.execute(CreateContainerCmdExec.java:13)
                at org.testcontainers.shaded.com.github.dockerjava.core.exec.AbstrSyncDockerCmdExec.exec(AbstrSyncDockerCmdExec.java:21)
                at org.testcontainers.shaded.com.github.dockerjava.core.command.AbstrDockerCmd.exec(AbstrDockerCmd.java:35)
                at org.testcontainers.shaded.com.github.dockerjava.core.command.CreateContainerCmdImpl.exec(CreateContainerCmdImpl.java:595)
                at org.testcontainers.utility.ResourceReaper.start(ResourceReaper.java:89)
                at org.testcontainers.DockerClientFactory.client(DockerClientFactory.java:201)
                at org.testcontainers.LazyDockerClient.getDockerClient(LazyDockerClient.java:14)
                at org.testcontainers.LazyDockerClient.listImagesCmd(LazyDockerClient.java:12)
                at org.testcontainers.images.LocalImagesCache.maybeInitCache(LocalImagesCache.java:68)
                at org.testcontainers.images.LocalImagesCache.get(LocalImagesCache.java:32)
                at org.testcontainers.images.AbstractImagePullPolicy.shouldPull(AbstractImagePullPolicy.java:18)
                at org.testcontainers.images.RemoteDockerImage.resolve(RemoteDockerImage.java:65)
                at org.testcontainers.images.RemoteDockerImage.resolve(RemoteDockerImage.java:26)
                at org.testcontainers.utility.LazyFuture.getResolvedValue(LazyFuture.java:17)
                at org.testcontainers.utility.LazyFuture.get(LazyFuture.java:39)
                at org.testcontainers.containers.GenericContainer.getDockerImageName(GenericContainer.java:1280)
                ... 4 more

1 test completed, 1 failed

iNikem · 2021-02-01T11:44:32Z

Do you have any local Docker setup/conf that could prevent from pulling images from Docker Hub?

…umentation into rxjava-2

piotr-sumo · 2021-02-01T11:50:42Z

I came across testcontainers/testcontainers-java#3574 and pulled that image manually. I've also merged latest main. Let's see what happens.
@iNikem I don't have such a docker config. Could you provide any or point me to the right direction?

iNikem · 2021-02-01T14:09:23Z

I came across testcontainers/testcontainers-java#3574 and pulled that image manually. I've also merged latest main. Let's see what happens.
@iNikem I don't have such a docker config. Could you provide any or point me to the right direction?

A, if this testcontainers bug (or we use too old version) and your local env works after pulling image manually, then problem solved, right?

piotr-sumo · 2021-02-01T14:14:33Z

@iNikem it is almost solved. I get

but when I open the file in Chrome I can see

anuraaga · 2021-02-02T03:56:44Z

instrumentation/rxjava-2.0/javaagent/rxjava-2.0-javaagent.gradle

+
+dependencies {
+  implementation group: 'org.reactivestreams', name: 'reactive-streams', version: '1.0.0'
+  library group: 'io.reactivex.rxjava2', name: 'rxjava', version: '2.0.0'


Does this instrumentation use rxjava, it seems to only instrument reactive streams? I couldn't find any usage outside of tests. If so we can name the instrumentation reactivestreams (we'd probably want to add tests with reactor too). Also I wonder how this instrumentation interops with other reactivestreams instrumentation we have, such as reactor.

Also for this instrumentation especially, I think it would help to start with library instrumentation instead of javaagent instrumentation - It uses so much bytecode instrumentation I don't think we could extract a library for it. An example library that instruments rxjava2 for context propagation is https://github.com/line/armeria/tree/master/rxjava2

I was porting DD instrumentation as mentioned here #1866. And the DD instrumentation uses javaagent instrumentation (https://github.com/DataDog/dd-trace-java/tree/master/dd-java-agent/instrumentation/rxjava-2)

@piotr-sumo Would you be able to help us by extracting this into library instrumentation? Going forward we do want to make sure we have it where we can. If it's too much for now though, I think this time we can stick with this and refactor (a fair amount I guess) later.

@anuraaga Sure, I'd like to contribute and extract this into library instrumentation. First, I'll familiarise myself with https://github.com/line/armeria/tree/master/rxjava2 and proceed with library instrumentation for rxjava-2.

piotr-sumo · 2021-02-05T10:12:57Z

RxJava2 library instrumentation PR can be found here #2191

…y-java-instrumentation into rxjava-2

iNikem · 2021-02-17T20:04:33Z

@piotr-sumo just to check, do you plan to finish this PR now when #2191 is merged?

piotr-sumo · 2021-02-18T08:49:49Z

@iNikem yes, I'm working on this now

piotr-sumo · 2021-02-19T07:35:49Z

@iNikem @mateuszrzeszutek @anuraaga could you review my changes?

anuraaga · 2021-02-19T08:55:00Z

@piotr-sumo Can you rework this to use the library instrumentation? We like the agent to just register library instrumentation whenever it can and it will make this a lot smaller. I think we can instrument RxJavaPlugins to enable our assembly - ideally we could instrument the static initializer of the class though I'm not sure I've seen us use that pattern. @trask do you know a good pattern for this?

piotr-sumo · 2021-02-19T09:13:02Z

@anuraaga the library instrumentation is already merged #2191
After reading the last comment in #1866 I've though that the javaagent instrumentation is still required.

However, I'm happy to contribute and I will rework implementation in this PR to meet your requirements.

anuraaga · 2021-02-19T09:15:16Z

@piotr-sumo Yup, it's great we have library instrumentation since now we can use it from this javaagent instrumentation :) We do need javaagent instrumentation but it should be implementable with a small amount of code that just initializes the library instrumentation instead of reimplementing it.

We use a similar pattern for reactor

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/reactor-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/reactor/ReactorHooksAdvice.java#L14

and it actually answered my question on how to run code post-static initializer :D

piotr-sumo · 2021-02-19T09:35:01Z

@anuraaga thanks for the hint! I will apply it!

piotr-sumo · 2021-02-19T15:29:31Z

@anuraaga I've applied your suggestion.

anuraaga

Thanks!

anuraaga · 2021-02-20T05:14:36Z

testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/asserts/TraceAssert.groovy

@@ -11,6 +11,7 @@ import groovy.transform.stc.ClosureParams
 import groovy.transform.stc.SimpleType
 import io.opentelemetry.instrumentation.testing.util.TelemetryDataUtil
 import io.opentelemetry.sdk.trace.data.SpanData
+


Can you revert this change?

piotr-sumo · 2021-02-22T07:37:44Z

@anuraaga @mateuszrzeszutek could you re-run the checks? It failed on part that is not related to my commit. Unfortunately I don't have permissions to trigger re-run myself.

mateuszrzeszutek

LGTM 👍

...ent/src/main/java/io/opentelemetry/instrumentation/rxjava2/RxJava2InstrumentationModule.java

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

piotr-sumo added 2 commits January 27, 2021 16:33

RxJava2 autoinstrumentation

77bc145

RxJava2 autoinstrumentation

356a611

piotr-sumo requested review from anuraaga, iNikem, jkwatson, mateuszrzeszutek, pavolloffay, trask and tylerbenson as code owners January 27, 2021 15:45

piotr-sumo mentioned this pull request Jan 28, 2021

Instrumentation for RxJava 2 #1866

Closed

Merge branch 'main' of github.com:piotr-sumo/opentelemetry-java-instr…

b4e900c

…umentation into rxjava-2

piotr-sumo added 4 commits February 1, 2021 15:17

Merge branch 'main' into rxjava-2

5dd9a70

Fixing gradle dependencies

87e00c6

Fixing codenarc violations

082df1d

Fixing spotless violations

5b1eb21

anuraaga reviewed Feb 2, 2021

View reviewed changes

piotr-sumo mentioned this pull request Feb 11, 2021

Rxjava2 library instrumentation #2191

Merged

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

9e4c047

…y-java-instrumentation into rxjava-2

piotr-sumo added 2 commits February 18, 2021 08:34

After merging with main

8773f8b

Applying spotless

e388eb2

piotr-sumo added 2 commits February 19, 2021 15:46

Javaagent uses library instrumentation

9e1ebef

Code cleanup

bdab49c

anuraaga approved these changes Feb 20, 2021

View reviewed changes

Revert changes to TraceAssert

c2635b1

mateuszrzeszutek approved these changes Feb 22, 2021

View reviewed changes

...ent/src/main/java/io/opentelemetry/instrumentation/rxjava2/RxJava2InstrumentationModule.java Outdated Show resolved Hide resolved

Hard-code class name

842cd98

Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>

mateuszrzeszutek approved these changes Feb 22, 2021

View reviewed changes

Spotless fix

6b8877a

mateuszrzeszutek merged commit b3504a1 into open-telemetry:main Feb 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rxjava 2 #2130

Rxjava 2 #2130

piotr-sumo commented Jan 27, 2021

linux-foundation-easycla bot commented Jan 27, 2021 •

edited

piotr-sumo commented Jan 28, 2021

jkwatson commented Jan 29, 2021

iNikem commented Jan 29, 2021

piotr-sumo commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021 •

edited

anuraaga commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021

mateuszrzeszutek commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021

iNikem commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021

iNikem commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021

anuraaga Feb 2, 2021

piotr-sumo Feb 2, 2021

anuraaga Feb 3, 2021

piotr-sumo Feb 3, 2021

piotr-sumo commented Feb 5, 2021

iNikem commented Feb 17, 2021

piotr-sumo commented Feb 18, 2021

piotr-sumo commented Feb 19, 2021

anuraaga commented Feb 19, 2021

piotr-sumo commented Feb 19, 2021

anuraaga commented Feb 19, 2021

piotr-sumo commented Feb 19, 2021

piotr-sumo commented Feb 19, 2021

anuraaga left a comment

anuraaga Feb 20, 2021

piotr-sumo Feb 21, 2021

piotr-sumo commented Feb 22, 2021

mateuszrzeszutek left a comment

Rxjava 2 #2130

Rxjava 2 #2130

Conversation

piotr-sumo commented Jan 27, 2021

linux-foundation-easycla bot commented Jan 27, 2021 • edited

piotr-sumo commented Jan 28, 2021

jkwatson commented Jan 29, 2021

iNikem commented Jan 29, 2021

piotr-sumo commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021 • edited

anuraaga commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021

mateuszrzeszutek commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021

iNikem commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021

iNikem commented Feb 1, 2021

piotr-sumo commented Feb 1, 2021

anuraaga Feb 2, 2021

Choose a reason for hiding this comment

piotr-sumo Feb 2, 2021

Choose a reason for hiding this comment

anuraaga Feb 3, 2021

Choose a reason for hiding this comment

piotr-sumo Feb 3, 2021

Choose a reason for hiding this comment

piotr-sumo commented Feb 5, 2021

iNikem commented Feb 17, 2021

piotr-sumo commented Feb 18, 2021

piotr-sumo commented Feb 19, 2021

anuraaga commented Feb 19, 2021

piotr-sumo commented Feb 19, 2021

anuraaga commented Feb 19, 2021

piotr-sumo commented Feb 19, 2021

piotr-sumo commented Feb 19, 2021

anuraaga left a comment

Choose a reason for hiding this comment

anuraaga Feb 20, 2021

Choose a reason for hiding this comment

piotr-sumo Feb 21, 2021

Choose a reason for hiding this comment

piotr-sumo commented Feb 22, 2021

mateuszrzeszutek left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jan 27, 2021 •

edited

piotr-sumo commented Feb 1, 2021 •

edited