From da8c331c1fa77271c66ae50fab1d4f4dcd05ae69 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Fri, 1 May 2020 18:51:12 -0600 Subject: [PATCH 1/3] Add more documentation for the instrumentor base class Fixes #637 --- .../auto_instrumentation/instrumentor.py | 41 ++++++++++++++++--- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py b/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py index f5d7cf7ddc2..79ac790ba9e 100644 --- a/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py +++ b/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py @@ -24,7 +24,17 @@ class BaseInstrumentor(ABC): - """An ABC for instrumentors""" + """An ABC for instrumentors + + Child classes of this ABC are implemented to instrument specific third + party libraries or frameworks either by using the + ``opentelemetry-auto-instrumentation`` command or by calling their methods + directly. + + Since every third party library or framework is different and has different + instrumentation needs, more methods can be added to the child classes as + needed to provide practical instrumentation to the end user. + """ _instance = None _is_instrumented = False @@ -38,14 +48,35 @@ def __new__(cls): @abstractmethod def _instrument(self, **kwargs): - """Instrument""" + """Instrument the library""" @abstractmethod def _uninstrument(self, **kwargs): - """Uninstrument""" + """Uninstrument the library""" def instrument(self, **kwargs): - """Instrument""" + """Instrument the library + + This method will be called without any optional arguments by the + ``opentelemetry-auto-instrumentation`` command. The configuration of + the instrumentation when done in this way should be done by previously + setting the configuration (using environment variables or any other + mechanism) that will be used later by the code in the ``instrument`` + implementation via the global ``Configuration`` object. + + When this method is to be called directly in the user code, ``kwargs`` + should be used to pass attributes that will override the configuration + values read by the ``Configuration`` object. + + In this way, calling this method directly without passing any optional + values should do the very same thing that the + ``opentelemetry-auto-instrumentation`` command does. The idea behind + this approach is also to keep the ``Configuration`` object immutable. + + This is necessary because this object is used by all the OpenTelemetry + components and any change to one of its attributes could break another + component, leading to very hard to debug bugs. + """ if not self._is_instrumented: result = self._instrument(**kwargs) @@ -57,7 +88,7 @@ def instrument(self, **kwargs): return None def uninstrument(self, **kwargs): - """Uninstrument""" + """Uninstrument the library""" if self._is_instrumented: result = self._uninstrument(**kwargs) From 0856702fa239de3d23ac75cf50b67190b1867593 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Mon, 4 May 2020 07:17:04 -0600 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Yusuke Tsutsumi --- .../src/opentelemetry/auto_instrumentation/instrumentor.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py b/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py index 79ac790ba9e..a77321fb4ba 100644 --- a/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py +++ b/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py @@ -26,7 +26,7 @@ class BaseInstrumentor(ABC): """An ABC for instrumentors - Child classes of this ABC are implemented to instrument specific third + Child classes of this ABC should instrument specific third party libraries or frameworks either by using the ``opentelemetry-auto-instrumentation`` command or by calling their methods directly. From 8957f412ff59ecece1e039da34c3f972d19e09ef Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Mon, 4 May 2020 07:50:18 -0600 Subject: [PATCH 3/3] Fix documentation --- .../opentelemetry/configuration/__init__.py | 11 +++++++- .../auto_instrumentation/instrumentor.py | 25 +++++++++---------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/opentelemetry-api/src/opentelemetry/configuration/__init__.py b/opentelemetry-api/src/opentelemetry/configuration/__init__.py index 57b1c324c6c..7d03c2f6157 100644 --- a/opentelemetry-api/src/opentelemetry/configuration/__init__.py +++ b/opentelemetry-api/src/opentelemetry/configuration/__init__.py @@ -74,9 +74,18 @@ To use the meter provider above, then the ``OPENTELEMETRY_PYTHON_METER_PROVIDER`` should be set to -"default_meter_provider" (this is not actually necessary since the +``"default_meter_provider"`` (this is not actually necessary since the OpenTelemetry API provided providers are the default ones used if no configuration is found in the environment variables). + +This object can be used by any OpenTelemetry component, native or external. +For that reason, the ``Configuration`` object is designed to be immutable. +If a component would change the value of one of the ``Configuration`` object +attributes then another component that relied on that value may break, leading +to bugs that are very hard to debug. To avoid this situation, the preferred +approach for components that need a different value than the one provided by +the ``Configuration`` object is to implement a mechanism that allows the user +to override this value instead of changing it. """ from os import environ diff --git a/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py b/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py index a77321fb4ba..dd58775bc7e 100644 --- a/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py +++ b/opentelemetry-auto-instrumentation/src/opentelemetry/auto_instrumentation/instrumentor.py @@ -64,18 +64,13 @@ def instrument(self, **kwargs): mechanism) that will be used later by the code in the ``instrument`` implementation via the global ``Configuration`` object. - When this method is to be called directly in the user code, ``kwargs`` - should be used to pass attributes that will override the configuration - values read by the ``Configuration`` object. - - In this way, calling this method directly without passing any optional - values should do the very same thing that the - ``opentelemetry-auto-instrumentation`` command does. The idea behind - this approach is also to keep the ``Configuration`` object immutable. - - This is necessary because this object is used by all the OpenTelemetry - components and any change to one of its attributes could break another - component, leading to very hard to debug bugs. + The ``instrument`` methods ``kwargs`` should default to values from the + ``Configuration`` object. + + This means that calling this method directly without passing any + optional values should do the very same thing that the + ``opentelemetry-auto-instrumentation`` command does. This approach is + followed because the ``Configuration`` object is immutable. """ if not self._is_instrumented: @@ -88,7 +83,11 @@ def instrument(self, **kwargs): return None def uninstrument(self, **kwargs): - """Uninstrument the library""" + """Uninstrument the library + + See ``BaseInstrumentor.instrument`` for more information regarding the + usage of ``kwargs``. + """ if self._is_instrumented: result = self._uninstrument(**kwargs)