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

[RFC] Update Flask plugin to allow existing Tracer instance re-use #726

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Kami
Copy link

@Kami Kami commented Jul 23, 2019

Background, Details

Right now Flask middleware / plugin creates new Tracer instance for each request.

This it not great when you are using the same tracer singleton instance across the whole application / service and when using other plugins (such as https://github.com/census-instrumentation/opencensus-python/blob/master/contrib/opencensus-ext-google-cloud-clientlibs/) which allow you to specify which tracer instance to use (e.g. using config_integration.trace_integrations([...], tracer=my_singleton_tracer_instance)).

Because existing singleton tracer instance can't re re-used, this means libraries and code which use singleton tracer instance can't be part of the root span created by the Flask middleware.

Before my change:

Screenshot from 2019-07-23 23-00-56

Screenshot from 2019-07-23 23-08-03

As you can see, span created by opencensus-ext-google-cloud-clientlibs which uses application / service global Tracer instance is not part of the request span (but I want it to be).


After my change:

Screenshot from 2019-07-23 23-00-43

I pass existing singleton tracer instance to Flask middleware and as such, span is correctly part of the root request span,

Proposed Solution

To solve for that, I propose adding new optional tracer argument to the constructor.

When this argument is provided, sampler, exporter and propagator arguments are redundant and mutually exclusive (if agreed on this approach, I can update docs and code to throw if mutually exclusive arguments are provided).

TODO

  • Update docs
  • Add tests
  • Update code to throw if mutually exclusive arguments are provided

@Kami Kami requested review from c24t, reyang, songy23 and a team as code owners July 23, 2019 21:10
Copy link
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reasonable workaround for the legacy design debt.

@c24t
Copy link
Member

c24t commented Jul 24, 2019

This looks like a good solution to me too pending the TODOs in the description.

constructor if mutually exclusive arguments are provided.
@Kami
Copy link
Author

Kami commented Jul 24, 2019

I pushed a change which adds docstring, test cases and throws inside a constructor if mutually exclusive arguments are provided.

I believe the tests / cover target should pass now.

@reyang
Copy link
Contributor

reyang commented Jul 24, 2019

@Kami would you help to update the CHANGELOG.md file given this is an interface change?
FYI - we plan to release around end of this month.

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

Successfully merging this pull request may close these issues.

4 participants