kezhenxu94 commented on a change in pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#discussion_r527702904
##########
File path: skywalking/plugins/sw_urllib_request.py
##########
@@ -38,15 +39,23 @@ def _sw_open(this: OpenerDirector, fullurl, data, timeout):
context = get_context()
carrier = Carrier()
- with context.new_exit_span(op=fullurl.selector or '/',
peer=fullurl.host, carrier=carrier) as span:
+ url = fullurl.selector.split("?")[0] if fullurl.selector else '/'
+ with context.new_exit_span(op=url, peer=fullurl.host, carrier=carrier)
as span:
span.layer = Layer.Http
span.component = Component.General
[fullurl.add_header(item.key, item.val) for item in carrier]
- res = _open(this, fullurl, data, timeout)
- span.tag(Tag(key=tags.HttpMethod, val=fullurl.get_method()))
- span.tag(Tag(key=tags.HttpUrl, val=fullurl.full_url))
+ try:
+ res = _open(this, fullurl, data, timeout)
+ span.tag(Tag(key=tags.HttpMethod, val=fullurl.get_method()))
+ except HTTPError as e:
+ span.tag(Tag(key=tags.HttpStatus, val=e.code))
+ raise
+ finally: # we do this here because it may change in _open()
+ span.tag(Tag(key=tags.HttpMethod, val=fullurl.get_method()))
Review comment:
> You are right I forgot to remove that, but I don't think that is the
source of the problem (or at least the only problem), lets see...
What potential problem do you foresee? From the code level, I think it looks
good. As the test case is simple (endpoint names without `?`), so the changes
should be OK IMO
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
