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

ext: Expect tracer provider instead of tracer in integrations #602

Merged
merged 14 commits into from
Apr 23, 2020
Merged

ext: Expect tracer provider instead of tracer in integrations #602

merged 14 commits into from
Apr 23, 2020

Conversation

mauriciovasquezbernal
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal commented Apr 20, 2020

Fixes #585

@lzchen @hectorhdzg I would love to get your opinions on this as it mainly affects the DB integrations that you worked on.

The requests integration is updated in #597, there are some fixes coming for Flask as well in #601, so I don't update them to avoid conflicts.

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

LGTM, I think this is more in line with the intention of "named tracers" than the original interfaces. 👍

Comment on lines 35 to 38
if tracer_provider is None:
tracer_provider = trace.get_tracer_provider()

tracer = tracer_provider.get_tracer(__name__, __version__)
Copy link
Member

Choose a reason for hiding this comment

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

Since this pattern will occur more often, would it make sense to add an utility function setup_tracer(tracer_provider: Optional[TracerProvider], name, version) to the API to encapsulate these three lines?

Copy link
Member Author

@mauriciovasquezbernal mauriciovasquezbernal Apr 21, 2020

Choose a reason for hiding this comment

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

I was thinking if it could make sense to add an optional TracerProvider parameter to get_tracer(). It'll be the same setup_tracer you're proposing.

Copy link
Contributor

@codeboten codeboten 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 good cleanup and gives us a pattern moving forward! Just a couple of non blocking questions.

"""Integrate with MySQL Connector/Python library.
https://dev.mysql.com/doc/connector-python/en/
"""

if tracer_provider is None:
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to call the original db_integration with the tracer_provider as a parameter since it's doing the same check?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that it'll create the tracer with the version and name from the db-api module. I had to split the logic from opentelemetry.ext.dbapi.trace_integration into two different functions, one receiving a tracer provider (to be used by the user), and other one receiving a tracer (to be used by other integrations).

@toumorokoshi
Copy link
Member

@mauriciovasquezbernal do you want to update the API to support that get_tracer pattern? Otherwise we can merge this in and take that as an improvement later.

@mauriciovasquezbernal
Copy link
Member Author

@toumorokoshi I implemented the change, would you mind taking a look?

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@@ -71,7 +103,7 @@ def trace_integration(
"""

# pylint: disable=unused-argument
def wrap_connect(
def wrap_connect_(
Copy link
Member

Choose a reason for hiding this comment

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

maybe a slight style thing, but it's a little harder for me to spot a trailing underscore rather than a leading underscore.

@toumorokoshi toumorokoshi merged commit 7cb57c7 into open-telemetry:master Apr 23, 2020
codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Apr 23, 2020
…elemetry#602)

Standardize the interface that trace providers are specified in integrations, as specified in open-telemetry#585.

Adding a helper to create and return a configured TracerProvider with a the span
processor and the memory exporter

api: Add tracer provider parameter to trace.get_tracer(). This eliminates the need for a helper function and boilerplate code to retrieve the appropriate tracer from a passed tracer_provider.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define how integrations should obtain a tracer
4 participants