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

gRPC instrumentation not checking for suppress_instrumentation in context #476

Open
aabmass opened this issue Apr 27, 2021 · 6 comments
Open
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed instrumentation triaged

Comments

@aabmass
Copy link
Member

aabmass commented Apr 27, 2021

opentelemetry-instrumentation-grpc is not checking for the special suppress_instrumentation context key before creating spans. This causes a recursion loop and stack overflow if you use SimpleSpanProcessor and a gRPC based exporter (like OTLP or Cloud Monitoring).

Describe your environment
opentelemetry-instrumentation-grpc 0.20b0

Steps to reproduce
Using SimpleSpanProcessor and OTLP exporter

What is the expected behavior?
Spans should not be created while suppress_instrumentation is in context

What is the actual behavior?
Spans get created and it create noise or causes stack overflow

@aabmass aabmass added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed instrumentation labels Apr 27, 2021
@ymotongpoo
Copy link
Contributor

ymotongpoo commented Apr 28, 2021

I faced this issue on calling Cloud Trace API in gRPC via opentelemetry-exporter-gcp-trace==1.0.0rc0. I didn't faced the issue when I just used opentelemetry-instrumentation-flask==0.20b0 for app instrumentation, and as soon as I introduced opentelemetry-instrumentation-grpc==0.20b0 on top of it, I faced this. The stack trace is here:
https://pastebin.com/7HqbHLj4

@NickSulistio
Copy link
Contributor

Hi! I would like to pick this up, but are there any good starting references for me to look at?

@lzchen
Copy link
Contributor

lzchen commented May 4, 2021

@NickSulistio
I believe the issue is simply that the opentelemetry-instrumentation-grpc does not check the context before deciding whether to create spans or not. Take a look at requests which does something similar.

@RyanSiu1995
Copy link
Contributor

The client side has been implemented by #559
I will handle the server side with another PR.

@RyanSiu1995
Copy link
Contributor

Before going to actual implementation, I want to make the clarification in the server side checking.
There are two ways for us to setup the Context.

  1. From propagator

Here the propagator is initiated by the client side so once the server side receives the key in the header. It will suppress the instrumentation.

  1. From the beginning in the server side

For the server side, I am thinking of how we can make the method-based suppress.
If we do sth like the following,

 def SimpleMethod(self, request, context):
        token = context.attach(
            set_context_value(_SUPPRESS_INSTRUMENTATION_KEY, True)
        )
        try:
            return Response(
                server_id=request.client_id, response_data=request.request_data,
            )
        finally:
            context.detach(token)

This will not work because the instrumentation happens from the interceptor. Before setting up the key, the instrumentation has been started.

I wonder which way the instrumentor should follow 🤔 . I am not 100% sure about this and the proposed behavior.

@RyanSiu1995
Copy link
Contributor

Hi @aabmass and @lzchen, do you have any idea on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed instrumentation triaged
Projects
None yet
Development

No branches or pull requests

5 participants