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.

Sign up for GitHub

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

Remove Configuration from instrumentations #285

Merged
merged 11 commits into from
Feb 4, 2021
Merged

Remove Configuration from instrumentations #285

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1092991
Remove Configuration
ocelotl Jan 10, 2021
d538042
Fix Changelog
ocelotl Jan 16, 2021
fd77085
Add HTTP util
ocelotl Jan 18, 2021
8a9e37a
Some fixes
ocelotl Jan 28, 2021
e45d3f4
Use environment variable from module
ocelotl Jan 28, 2021
f005d90
Fix lint
ocelotl Jan 29, 2021
5f9bc0e
Downgrade version to clear cache and make docs tests pass
ocelotl Jan 29, 2021
3014921
Another try
ocelotl Jan 30, 2021
e6dfad1
Update workflow
ocelotl Jan 30, 2021
0b0705e
Fix docs
ocelotl Jan 30, 2021
602dca3
Put asgi and wsgi back as instrumentations
ocelotl Feb 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ on:
- 'release/*'
pull_request:
env:
CORE_REPO_SHA: f3ee81243b4266729ba5196a7883ce897549aaba
CORE_REPO_SHA: 09b010cfcc85e2aa07326e9204541b80a7dd52f0

jobs:
build:
Expand Down
1 change: 1 addition & 0 deletions .pylintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ disable=missing-docstring,
super-init-not-called, # temp-pylint-upgrade
invalid-overridden-method, # temp-pylint-upgrade
missing-module-docstring, # temp-pylint-upgrade
import-error, # needed as a workaround as reported here: https://github.com/open-telemetry/opentelemetry-python-contrib/issues/290

# Enable the message, report, category or checker with the given id(s). You can
# either give multiple identifier separated by comma (,) or put this option
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Update TraceState to adhere to specs
([#276](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/276))

### Removed
- Remove Configuration
([#285](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/285))

## [0.16b1](https://github.com/open-telemetry/opentelemetry-python-contrib/releases/tag/v0.16b1) - 2020-11-26

## [0.16b0](https://github.com/open-telemetry/opentelemetry-python-contrib/releases/tag/v0.16b0) - 2020-11-25
Expand Down
7 changes: 0 additions & 7 deletions docs-requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,11 @@ sphinx~=2.4
sphinx-rtd-theme~=0.4
sphinx-autodoc-typehints~=1.10.2

# Need to install the api/sdk in the venv for autodoc. Modifying sys.path
# doesn't work for pkg_resources.
-e "git+https://github.com/open-telemetry/opentelemetry-python.git#egg=opentelemetry-api&subdirectory=opentelemetry-api"
-e "git+https://github.com/open-telemetry/opentelemetry-python.git#egg=opentelemetry-sdk&subdirectory=opentelemetry-sdk"

# Required by opentelemetry-instrumentation
fastapi~=0.58.1
psutil~=5.7.0
pymemcache~=1.3

-e "git+https://github.com/open-telemetry/opentelemetry-python.git#egg=opentelemetry-instrumentation&subdirectory=opentelemetry-instrumentation"

# Required by conf
django>=2.2

Expand Down
9 changes: 0 additions & 9 deletions docs/instrumentation/asgi/asgi.rst
View file
Open in desktop

This file was deleted.

7 changes: 0 additions & 7 deletions docs/instrumentation/wsgi/wsgi.rst
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion docs/nitpick-exceptions.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ class_references=
opentelemetry.trace.propagation.textmap.TextMapPropagator
; - AwsXRayFormat
opentelemetry.trace.propagation.textmap.DictGetter
; - instrumentation.asgi.CarrierGetter
; API
opentelemetry.trace.propagation.textmap.Getter
; - DatadogFormat
Expand Down
3 changes: 1 addition & 2 deletions eachdist.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@ ignore=
opentelemetry-python-core

sortfirst=
instrumentation/opentelemetry-instrumentation-wsgi
util/opentelemetry-util-http
instrumentation/opentelemetry-instrumentation-dbapi
instrumentation/opentelemetry-instrumentation-asgi
instrumentation/opentelemetry-instrumentation-botocore
instrumentation/*
exporter/*
Expand Down
2 changes: 1 addition & 1 deletion exporter/opentelemetry-exporter-datadog/src/opentelemetry/exporter/datadog/exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
"opentelemetry.instrumentation.redis": DatadogSpanTypes.REDIS,
"opentelemetry.instrumentation.requests": DatadogSpanTypes.HTTP,
"opentelemetry.instrumentation.sqlalchemy": DatadogSpanTypes.SQL,
"opentelemetry.instrumentation.wsgi": DatadogSpanTypes.WEB,
"opentelemetry.util.http.wsgi": DatadogSpanTypes.WEB,
}


Expand Down
2 changes: 1 addition & 1 deletion exporter/opentelemetry-exporter-datadog/tests/test_datadog_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ def test_span_types(self):
"opentelemetry.instrumentation.redis",
"opentelemetry.instrumentation.requests",
"opentelemetry.instrumentation.sqlalchemy",
"opentelemetry.instrumentation.wsgi",
"opentelemetry.util.http.wsgi",
]

for index, instrumentation in enumerate(test_instrumentations):
Expand Down
2 changes: 0 additions & 2 deletions ...ion/opentelemetry-instrumentation-boto/src/opentelemetry/instrumentation/boto/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,6 @@ def _instrument(self, **kwargs):
# For exemple EC2 uses AWSQueryConnection and S3 uses
# AWSAuthConnection

# FIXME should the tracer provider be accessed via Configuration
# instead?
# pylint: disable=attribute-defined-outside-init
self._tracer = get_tracer(
__name__, __version__, kwargs.get("tracer_provider")
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/opentelemetry-instrumentation-django/setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ package_dir=
packages=find_namespace:
install_requires =
django >= 1.10
opentelemetry-instrumentation-wsgi == 0.18.dev0
opentelemetry-util-http == 0.18.dev0
opentelemetry-instrumentation == 0.18.dev0
opentelemetry-api == 0.18.dev0

Expand Down
11 changes: 5 additions & 6 deletions ...opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@
# limitations under the License.

from logging import getLogger
from os import environ

from django.conf import settings

from opentelemetry.configuration import Configuration
from opentelemetry.instrumentation.django.environment_variables import (
OTEL_PYTHON_DJANGO_INSTRUMENT,
)
from opentelemetry.instrumentation.django.middleware import _DjangoMiddleware
from opentelemetry.instrumentation.django.version import __version__
from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
Expand All @@ -43,11 +46,7 @@ def _instrument(self, **kwargs):

# FIXME this is probably a pattern that will show up in the rest of the
# ext. Find a better way of implementing this.
# FIXME Probably the evaluation of strings into boolean values can be
# built inside the Configuration class itself with the magic method
# __bool__

if Configuration().DJANGO_INSTRUMENT is False:
if environ.get(OTEL_PYTHON_DJANGO_INSTRUMENT) == "False":
return

# This can not be solved, but is an inherent problem of this approach:
Expand Down
13 changes: 1 addition & 12 deletions ...entelemetry-instrumentation-asgi/setup.py → ...mentation/django/environment_variables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,5 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import os

import setuptools

BASE_DIR = os.path.dirname(__file__)
VERSION_FILENAME = os.path.join(
BASE_DIR, "src", "opentelemetry", "instrumentation", "asgi", "version.py"
)
PACKAGE_INFO = {}
with open(VERSION_FILENAME) as f:
exec(f.read(), PACKAGE_INFO)

setuptools.setup(version=PACKAGE_INFO["__version__"])
OTEL_PYTHON_DJANGO_INSTRUMENT = "OTEL_PYTHON_DJANGO_INSTRUMENT"
27 changes: 13 additions & 14 deletions ...entelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,22 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import time
from logging import getLogger
from time import time

from django.conf import settings

from opentelemetry.configuration import Configuration
from opentelemetry.context import attach, detach
from opentelemetry.instrumentation.django.version import __version__
from opentelemetry.instrumentation.utils import extract_attributes_from_object
from opentelemetry.instrumentation.wsgi import (
Copy link
Member

Choose a reason for hiding this comment

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

I believe this wsgi instrumentation library could be used for any wsgi framework to collect telemetry. For instance let's say I have a WSGI based framework X and there is no standalone instrumentation for it. If we remove this what would be the way for collecting telemetry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The WSGI code as it is now is not an instrumentation because it does no implement BaseInstrumentor. There is no WSGI library that is instrumented by it as opposed to specific libraries like Flask, Django, etc. For that reason, the WSGI code should not be in opentelemetry.instrumentation.

I don't understand what you mean with "For instance let's say I have a WSGI based framework X and there is no standalone instrumentation for it. If we remove this what would be the way for collecting telemetry?", can you explain, please?

Copy link
Member

@srikanthccv srikanthccv Jan 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The WSGI and ASGI are different instrumentation libraries on their own as of now. I may be wrong but my understanding is that I can use opentelemetry-instrumentation-wsgi with any wsgi based web frameworks to collect telemetry. I will try to use an example to clarify the last part, We don't have the instrumentation library for cherrpy framework but I can still instrument my cherrpy based application using opentelemetry-instrumentation-wsgi. It would look something similar as followed

import cherrypy
from opentelemetry.instrumentation.wsgi import OpenTelemetryMiddleware

class HelloWorld(object):
    @cherrypy.expose
    def index(self):
        return "Hello World!"

app  = cherrypy.Application(HelloWorld(), '/')

app.wsgiapp = OpenTelemetryMiddleware(app.wsgiapp)

cherrypy.quickstart(app)

I didn't think of WSGI/ASGI OpenTelemetryMiddleware as an util although there are some util functions in that package. It was easier to find that library, docs and use because it is very similar to any other popular middlewares flask-cache or bottle middlewares etc.. to name few. I thought may be util is not an ideal place to put middleware kind of stuff, just wanted to understand the motivation behind moving from individual packages to util. On the other hand I think it is good to move the helper functions under util.http.

from opentelemetry.propagators import extract
from opentelemetry.trace import SpanKind, get_tracer
from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs
from opentelemetry.util.http.wsgi import (
add_response_attributes,
carrier_getter,
collect_request_attributes,
)
from opentelemetry.propagators import extract
from opentelemetry.trace import SpanKind, get_tracer

try:
from django.core.urlresolvers import ( # pylint: disable=no-name-in-module
Expand Down Expand Up @@ -61,9 +61,8 @@ class _DjangoMiddleware(MiddlewareMixin):
_environ_span_key = "opentelemetry-instrumentor-django.span_key"
_environ_exception_key = "opentelemetry-instrumentor-django.exception_key"

_excluded_urls = Configuration()._excluded_urls("django")

_traced_request_attrs = Configuration()._traced_request_attrs("django")
_traced_request_attrs = get_traced_request_attrs("DJANGO")
_excluded_urls = get_excluded_urls("DJANGO")

@staticmethod
def _get_span_name(request):
Expand Down Expand Up @@ -111,23 +110,23 @@ def process_request(self, request):
return

# pylint:disable=W0212
request._otel_start_time = time.time()
request._otel_start_time = time()

environ = request.META
request_meta = request.META

token = attach(extract(carrier_getter, environ))
token = attach(extract(carrier_getter, request_meta))

tracer = get_tracer(__name__, __version__)

span = tracer.start_span(
self._get_span_name(request),
kind=SpanKind.SERVER,
start_time=environ.get(
start_time=request_meta.get(
"opentelemetry-instrumentor-django.starttime_key"
),
)

attributes = collect_request_attributes(environ)
attributes = collect_request_attributes(request_meta)
# pylint:disable=W0212
request._otel_labels = self._get_metric_labels_from_attributes(
attributes
Expand Down Expand Up @@ -215,7 +214,7 @@ def process_response(self, request, response):
if metric_recorder is not None:
# pylint:disable=W0212
metric_recorder.record_server_duration_range(
request._otel_start_time, time.time(), request._otel_labels
request._otel_start_time, time(), request._otel_labels
)
except Exception as ex: # pylint: disable=W0703
_logger.warning("Error recording duration metrics: %s", ex)
Expand Down
7 changes: 3 additions & 4 deletions instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@
from django.test import Client
from django.test.utils import setup_test_environment, teardown_test_environment

from opentelemetry.configuration import Configuration
from opentelemetry.instrumentation.django import DjangoInstrumentor
from opentelemetry.sdk.util import get_dict_as_key
from opentelemetry.test.test_base import TestBase
from opentelemetry.test.wsgitestutil import WsgiTestBase
from opentelemetry.trace import SpanKind
from opentelemetry.trace.status import StatusCode
from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs

# pylint: disable=import-error
from .views import (
Expand Down Expand Up @@ -64,7 +64,6 @@ def setUp(self):
super().setUp()
setup_test_environment()
_django_instrumentor.instrument()
Configuration._reset() # pylint: disable=protected-access
self.env_patch = patch.dict(
"os.environ",
{
Expand All @@ -75,11 +74,11 @@ def setUp(self):
self.env_patch.start()
self.exclude_patch = patch(
"opentelemetry.instrumentation.django.middleware._DjangoMiddleware._excluded_urls",
Configuration()._excluded_urls("django"),
get_excluded_urls("DJANGO"),
)
self.traced_patch = patch(
"opentelemetry.instrumentation.django.middleware._DjangoMiddleware._traced_request_attrs",
Configuration()._traced_request_attrs("django"),
get_traced_request_attrs("DJANGO"),
)
self.exclude_patch.start()
self.traced_patch.start()
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/opentelemetry-instrumentation-falcon/setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ package_dir=
packages=find_namespace:
install_requires =
falcon ~= 2.0
opentelemetry-instrumentation-wsgi == 0.18.dev0
opentelemetry-util-http == 0.18.dev0
opentelemetry-instrumentation == 0.18.dev0
opentelemetry-api == 0.18.dev0

Expand Down
19 changes: 10 additions & 9 deletions ...opentelemetry-instrumentation-falcon/src/opentelemetry/instrumentation/falcon/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

"""
This library builds on the OpenTelemetry WSGI middleware to track web requests
in Falcon applications. In addition to opentelemetry-instrumentation-wsgi,
in Falcon applications. In addition to opentelemetry-util-http,
it supports falcon-specific features such as:

* The Falcon resource and method name is used as the Span name.
Expand Down Expand Up @@ -43,14 +43,13 @@ def on_get(self, req, resp):
---
"""

import sys
from logging import getLogger
from sys import exc_info

import falcon

import opentelemetry.instrumentation.wsgi as otel_wsgi
from opentelemetry import configuration, context, propagators, trace
from opentelemetry.configuration import Configuration
import opentelemetry.util.http.wsgi as otel_wsgi
from opentelemetry import context, propagators, trace
from opentelemetry.instrumentation.falcon.version import __version__
from opentelemetry.instrumentation.instrumentor import BaseInstrumentor
from opentelemetry.instrumentation.utils import (
Expand All @@ -59,6 +58,7 @@ def on_get(self, req, resp):
)
from opentelemetry.trace.status import Status
from opentelemetry.util import time_ns
from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs

_logger = getLogger(__name__)

Expand All @@ -68,8 +68,9 @@ def on_get(self, req, resp):
_ENVIRON_TOKEN = "opentelemetry-falcon.token"
_ENVIRON_EXC = "opentelemetry-falcon.exc"

cfg = configuration.Configuration()
_excluded_urls = cfg._excluded_urls("falcon")

_excluded_urls = get_excluded_urls("FALCON")
_traced_request_attrs = get_traced_request_attrs("FALCON")


class FalconInstrumentor(BaseInstrumentor):
Expand Down Expand Up @@ -149,7 +150,7 @@ class _TraceMiddleware:

def __init__(self, tracer=None, traced_request_attrs=None):
self.tracer = tracer
self._traced_request_attrs = cfg._traced_request_attrs("falcon")
self._traced_request_attrs = _traced_request_attrs

def process_request(self, req, resp):
span = req.env.get(_ENVIRON_SPAN_KEY)
Expand Down Expand Up @@ -186,7 +187,7 @@ def process_response(
status = "404"
reason = "NotFound"

exc_type, exc, _ = sys.exc_info()
exc_type, exc, _ = exc_info()
if exc_type and not req_succeeded:
if "HTTPNotFound" in exc_type.__name__:
status = "404"
Expand Down
7 changes: 3 additions & 4 deletions instrumentation/opentelemetry-instrumentation-falcon/tests/test_falcon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@

from falcon import testing

from opentelemetry.configuration import Configuration
from opentelemetry.instrumentation.falcon import FalconInstrumentor
from opentelemetry.test.test_base import TestBase
from opentelemetry.trace.status import StatusCode
from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs

from .app import make_app

Expand All @@ -30,7 +30,6 @@ def setUp(self):
FalconInstrumentor().instrument()
self.app = make_app()
# pylint: disable=protected-access
Configuration()._reset()
self.env_patch = patch.dict(
"os.environ",
{
Expand All @@ -41,15 +40,15 @@ def setUp(self):
self.env_patch.start()
self.exclude_patch = patch(
"opentelemetry.instrumentation.falcon._excluded_urls",
Configuration()._excluded_urls("falcon"),
get_excluded_urls("FALCON"),
)
middleware = self.app._middleware[0][ # pylint:disable=W0212
0
].__self__
self.traced_patch = patch.object(
middleware,
"_traced_request_attrs",
Configuration()._traced_request_attrs("falcon"),
get_traced_request_attrs("FALCON"),
)
self.exclude_patch.start()
self.traced_patch.start()
Expand Down
Loading