Conditionally Creating Server Spans if No Span Found in Current Context

[andresbeckruiz]

Description

This PR fixes issue #445. I check if if an active span is present in the current context by calling opentelemetry.trace.get_current_span(). If one is found, an internal span is created and the span found is used as the parent. If one is not found, a server span is created and the remote span context from the incoming request is used as a parent.

I used either this logic for all of the server instrumentations that create server spans: Falcon, Flask, Django, Pyramid, Tornado, WSGI, and ASGI. There are references to the issue that mention that FastAPI and Starlette also create server spans, but I could not find where this occurs in the opentelemetry-python-contrib repo. If someone could point out where they are created, I can add the logic I used for the other instrumentations.

Type of change

Please delete options that are not relevant.

• [ ✅ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  -This feature causes server spans to be only created conditionally, so at times existing functionality will not work as expected.

How Has This Been Tested?

All the instrumentation tests were run and passed successfully.

Does This PR Require a Core Repo Change?

• Yes. - Link to PR:
• [ ✅ ] No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

• [ ✅ ] Followed the style guidelines of this project
• [ ✅ ] Changelogs have been updated
• [ ✅ ] Unit tests have been added
• [ ✅ ] Documentation has been updated

[ocelotl]

@andresbeckruiz please make sure that all tests pass.

[srikanthccv]

Two things on top of my mind that we probably should do

1. We want to continue the trace from a remote context. I think we should check that the trace id from the current span context is the same as the trace id from the incoming request before starting the INTERNAL span. Otherwise, this would lead to broken traces.
2. A check that parent span is SERVER span.

[owais]

1.0 We want to continue the trace from a remote context. I think we should check that the trace id from the current span context is the same as the trace id from the incoming request before starting the INTERNAL span. Otherwise, this would lead to broken traces.

I still cannot think of any good reason why there would be locally active span that wouldn't really be part of the same trace/request so not sure how much benefit this check will provide. It's certainly not harmful but may be we can avoid extra checks on potentially hotpaths if we can't think of a edge-case to catch with this.

2.0 A check that parent span is SERVER span.

I see how this makes sense but can there be situations where the real server span has a child span and then this logic is triggered? Let's say if WSGI is being used with Django and then a Django middleware embeds yet another router/server library that is also instrumented. The 3rd span would create another SERVER span in that case. May be we can keep it simple and just use the local span if present without any other conditions? We can easily add more checks later if we discover any cases where this doesn't work.

[srikanthccv]

Ah I see how this can get complicated quickly if we consider about all cases. Yes, maybe we can just keep it simple and revisit later if people run into these edge case scenarios.

[andresbeckruiz]

@owais I have updated each instrumentation file and addressed your comments.

[andresbeckruiz]

@ocelotl all tests are passing now for instrumentations that I have edited. For some reason, the lint and docker tests are failing still, but I don't believe it's because of my edits.

[andresbeckruiz]

@owais I have addressed your most recent comments.

[owais]

This contains commits from unrelated branches. Can you please rebase and perhaps squash the commits (just this time) so it is easier to review?

[andresbeckruiz]

@owais sorry about that, I created a new local branch to rebase and clean everything up and pushed it onto this branch. I also addressed your most recent comments.

[owais]

This works now but it looks like something that could easily break in future with the value of ctx determining whether token is defined or not. Why not have a none-check for token directly instead?

[owais]

Mostly looks good. Left some minor comments.

Major remaining blocker are tests. We need to ensure all known cases work as expected for all instrumentation that were touched. Can you please add tests for such cases?

[andresbeckruiz]

Hi @owais, I addressed the minor comments you left. I am stuck figuring out how to implement tests for the logic I have implemented because I am unsure how to access the spans being created. For example, in the Django middleware tests I cannot find any that call the process_request method that I have touched. I see tests accessing spans using a get_finished_spans method but I am not sure if that method uses the logic I updated. Furthermore, the process_request method does not return the span, context, or token, which I have all updated logic for. Therefore, I am not sure how to check for instance if the span created is of SpanKind.INTERNAL. Would you be able to give an example or point me in the right direction on how I could implement tests for the logic I have updated?

[owais]

I think existing tests should continue to work with the changes you made. You need to add new tests where a span is already present in the current context before the framework receives an HTTP request. You can do this manually but it'll probably differ across frameworks. It might be simpler to have WSGI instrumentation active as well in addition to the framework instrumentation for such tests as that'd automatically reproduce the scenario. Once that is done, your test for checking spans would look exactly the same as any other test.

[owais]

@andresbeckruiz Thanks for contributing this to the project. It's a very valuable feature and I really appreciate all the effort you put into this. Could you please clarify if you are planning to come back to this or if we should re-assign it to someone else so they can build on top of your work?

[andresbeckruiz]

Hi @owais sorry I did not clarify; I am pretty busy right now so I will not be coming back to this issue. I would appreciate if you could re-assign the issue to someone else so that they can build on top of my work, thank you!