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

Jump to bottom

Fix requests and urllib instrumentations span name callback parameters #259

Merged
merged 4 commits into from Dec 16, 2020
Merged

Fix requests and urllib instrumentations span name callback parameters #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d54bc5c
include
lzchen Dec 15, 2020
4587e20
fix
lzchen Dec 15, 2020
48d42d4
test
lzchen Dec 15, 2020
e4eb2ca
changelog
lzchen Dec 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -39,6 +39,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#242](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/242))
- `opentelemetry-instrumentation-flask` Do not emit a warning message for request contexts created with `app.test_request_context`
([#253](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/253))
- `opentelemetry-instrumentation-requests`, `opentelemetry-instrumentation-urllib` Fix span name callback parameters
- ([#259](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/259))
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that there's an extra dash on line 43 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably will be fixed in a new pr


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

Expand Down
2 changes: 1 addition & 1 deletion ...telemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py
View file
Open in desktop
Expand Up @@ -122,7 +122,7 @@ def _instrumented_requests_call(
method = method.upper()
span_name = ""
if name_callback is not None:
span_name = name_callback()
span_name = name_callback(method, url)
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought that although this is sufficient for the vast majority of cases, I have a feeling people may want to customize the span name further later on.

For example, one cannot extracted values from the header and add them to the span name.

I don't have a great solution (besides passing in all args via **kwargs, etc), but this may come up as an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I thought of that too. For now, I think this is sufficient and we can change the parameters if the need arises.

if not span_name or not isinstance(span_name, str):
span_name = get_default_span_name(method)

Expand Down
8 changes: 4 additions & 4 deletions instrumentation/opentelemetry-instrumentation-requests/tests/test_requests_integration.py
View file
Open in desktop
Expand Up @@ -108,19 +108,19 @@ def test_basic(self):
self.assertGreaterEqual(view_data.aggregator.current.sum, 0)

def test_name_callback(self):
def name_callback():
return "test_name"
def name_callback(method, url):
return "GET" + url

RequestsInstrumentor().uninstrument()
RequestsInstrumentor().instrument(name_callback=name_callback)
result = self.perform_request(self.URL)
self.assertEqual(result.text, "Hello!")
span = self.assert_span()

self.assertEqual(span.name, "test_name")
self.assertEqual(span.name, "GET" + self.URL)

def test_name_callback_default(self):
def name_callback():
def name_callback(method, url):
return 123

RequestsInstrumentor().uninstrument()
Expand Down
4 changes: 1 addition & 3 deletions ...opentelemetry-instrumentation-urllib/src/opentelemetry/instrumentation/urllib/__init__.py
View file
Open in desktop
Expand Up @@ -101,8 +101,6 @@ def uninstrument_opener(
_uninstrument_from(opener, restore_as_bound_func=True)


# pylint: disable=unused-argument
# pylint: disable=R0915
def get_default_span_name(method):
"""Default implementation for name_callback, returns HTTP {method_name}."""
return "HTTP {}".format(method).strip()
Expand Down Expand Up @@ -145,7 +143,7 @@ def _instrumented_open_call(

span_name = ""
if name_callback is not None:
span_name = name_callback()
span_name = name_callback(method, url)
lzchen marked this conversation as resolved.
Show resolved Hide resolved
if not span_name or not isinstance(span_name, str):
span_name = get_default_span_name(method)

Expand Down
8 changes: 4 additions & 4 deletions instrumentation/opentelemetry-instrumentation-urllib/tests/test_urllib_integration.py
View file
Open in desktop
Expand Up @@ -137,8 +137,8 @@ def test_basic(self):
self.assertGreaterEqual(view_data.aggregator.current.sum, 0)

def test_name_callback(self):
def name_callback():
return "test_name"
def name_callback(method, url):
return "GET" + url

URLLibInstrumentor().uninstrument()
URLLibInstrumentor().instrument(name_callback=name_callback)
Expand All @@ -147,10 +147,10 @@ def name_callback():
self.assertEqual(result.read(), b"Hello!")
span = self.assert_span()

self.assertEqual(span.name, "test_name")
self.assertEqual(span.name, "GET" + self.URL)

def test_name_callback_default(self):
def name_callback():
def name_callback(method, url):
return 123

URLLibInstrumentor().uninstrument()
Expand Down