Skip to content

Fix requests and urllib instrumentations span name callback parameters#259

Merged
lzchen merged 4 commits intoopen-telemetry:masterfrom
lzchen:hooks
Dec 16, 2020
Merged

Fix requests and urllib instrumentations span name callback parameters#259
lzchen merged 4 commits intoopen-telemetry:masterfrom
lzchen:hooks

Conversation

@lzchen
Copy link
Contributor

@lzchen lzchen commented Dec 15, 2020

Addresses this comment

@lzchen lzchen requested review from a team, hectorhdzg and toumorokoshi and removed request for a team December 15, 2020 18:28
@lzchen lzchen added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 15, 2020
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.

LGTM! Thanks.

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.

@toumorokoshi
Copy link
Member

QQ: why does this have the "Skip Changelog" tag? this is a new feature being added to the requests / urllib instrumentations. or at least a bugfix.

@lzchen lzchen removed the Skip Changelog PRs that do not require a CHANGELOG.md entry label Dec 16, 2020
@lzchen lzchen merged commit 65801c3 into open-telemetry:master Dec 16, 2020
@lzchen lzchen deleted the hooks branch December 16, 2020 16:17
- `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

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.

3 participants