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

Migrate to new OpenCensus APIs for OpenTelemetry migration #7732

Open
ejona86 opened this issue Dec 16, 2020 · 3 comments
Open

Migrate to new OpenCensus APIs for OpenTelemetry migration #7732

ejona86 opened this issue Dec 16, 2020 · 3 comments
Assignees
Milestone

Comments

@ejona86
Copy link
Member

ejona86 commented Dec 16, 2020

census-instrumentation/opencensus-java#2059 added some new APIs to be able to use the gRPC context or the OpenTelemetry context. That change breaks gRPC (census-instrumentation/opencensus-java#2069). Ignoring the breakage, we still want to figure out what changes we should make on our side and whether we can adopt the new API. We should also investigate what their migration looks like in the context of gRPC and its users.

Gribkoff, assigning to you for initial investigation, since this may influence your work. But I assume that before any larger changes we'd coordinate and determine who is appropriate.

@ejona86 ejona86 added this to the Next milestone Dec 16, 2020
ejona86 added a commit to ejona86/grpc-java that referenced this issue Dec 16, 2020
I didn't touch Protobuf and Netty; we upgrade those individually. Below
are issues I encountered that caused me to not upgrade (further).

Guava 30.1-android fails to build with Android without enabling
desugaring. google/guava#5358

Robolectric 4.4 breaks AndroidChannelBuilderTest.
grpc#7731

Opencensus 0.28.1+ is incompatible with gRPC.
census-instrumentation/opencensus-java#2069
grpc#7732

Truth now defines the asm dependency as "compile" although it is still
optional. But asm appears to have accidentally included incorrect gradle
module metadata in their release (I see they've disabled the metadata on
master) which make gradle think it requires Java 8. We could asm
everywhere, but that's is annoying. It seems likely this will resolve
itself.

Mockito can be upgraded to 3.4.0, but it deprecates initMocks, which
causes more code churn than I wanted in this commit. I still
synchronized the example versions on 3.4.0, though, as it was already
being used in some examples and the examples don't use initMocks.
ejona86 added a commit that referenced this issue Dec 17, 2020
I didn't touch Protobuf and Netty; we upgrade those individually. Below
are issues I encountered that caused me to not upgrade (further).

Guava 30.1-android fails to build with Android without enabling
desugaring. google/guava#5358

Robolectric 4.4 breaks AndroidChannelBuilderTest.
#7731

Opencensus 0.28.1+ is incompatible with gRPC.
census-instrumentation/opencensus-java#2069
#7732

Truth now defines the asm dependency as "compile" although it is still
optional. But asm appears to have accidentally included incorrect gradle
module metadata in their release (I see they've disabled the metadata on
master) which make gradle think it requires Java 8. We could asm
everywhere, but that's is annoying. It seems likely this will resolve
itself.

Mockito can be upgraded to 3.4.0, but it deprecates initMocks, which
causes more code churn than I wanted in this commit. I still
synchronized the example versions on 3.4.0, though, as it was already
being used in some examples and the examples don't use initMocks.
dfawley pushed a commit to dfawley/grpc-java that referenced this issue Jan 15, 2021
I didn't touch Protobuf and Netty; we upgrade those individually. Below
are issues I encountered that caused me to not upgrade (further).

Guava 30.1-android fails to build with Android without enabling
desugaring. google/guava#5358

Robolectric 4.4 breaks AndroidChannelBuilderTest.
grpc#7731

Opencensus 0.28.1+ is incompatible with gRPC.
census-instrumentation/opencensus-java#2069
grpc#7732

Truth now defines the asm dependency as "compile" although it is still
optional. But asm appears to have accidentally included incorrect gradle
module metadata in their release (I see they've disabled the metadata on
master) which make gradle think it requires Java 8. We could asm
everywhere, but that's is annoying. It seems likely this will resolve
itself.

Mockito can be upgraded to 3.4.0, but it deprecates initMocks, which
causes more code churn than I wanted in this commit. I still
synchronized the example versions on 3.4.0, though, as it was already
being used in some examples and the examples don't use initMocks.
@eugeniyk
Copy link

I can't find any example how to instrument java grpc with OpenTelemetry api
Do you know is there any open version for that? like we have in opentracing-contrib/java-grpc

The only example I found is autoinstrumentation via aspects in opentelemetry-java-instrumentation, but that's works via java agent

@trask
Copy link
Contributor

trask commented Nov 7, 2023

@ejona86 ejona86 assigned YifeiZhuang and DNVindhya and unassigned ericgribkoff Nov 7, 2023
@gavin-nia
Copy link

We just started migrating from opencensus to opentelemetry but grpc tracing does not work correctly.

My understanding is that the opentelemetry maintainers have provided a shim that allows libraries such as gRPC to continue to use opencensus APIs (the shim converts these to OT under the hood). This means you don't need to change the gRPC code and users of gRPC just need to add the shim as a dependency.

However, one problem is the usage of the gRPC context to store the current span etc. This is referenced in the following TODO (there may be others):

TODO(dnvindhya): Replace deprecated ContextUtils usage with ContextHandleUtils to interact

The issue we see is that when using the shim, the root server side span (called "Recv.something") is not correctly recognized as the parent of other spans added in user code in the server. I suppose that happens because gRPC directly stores the span in a grpc context, but OT wants to use its own context management.

Is there any chance that someone can look at this or give a little guidance on the minimal change to get things working?

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

No branches or pull requests

7 participants