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

[Plugin] http servers set URL and status tags #85

Merged
merged 5 commits into from
Nov 21, 2020
Merged

[Plugin] http servers set URL and status tags #85

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac6c22f
[Plugin] http servers set URL and status tags
tom-pytel Nov 20, 2020
586c3b6
[Plugin] made all status tags overridable
tom-pytel Nov 20, 2020
44a3a99
Fix: Span.tag() for overridable
tom-pytel Nov 20, 2020
8eb810a
updated tests for new tags
tom-pytel Nov 20, 2020
9675cae
Fix: _sw_open(), Span.__exit__()
tom-pytel Nov 20, 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: 1 addition & 1 deletion skywalking/plugins/sw_django.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def _sw_get_response(this, request):
val=params_tostring(request.GET)[0:config.http_params_length_threshold]))

resp = _get_response(this, request)
span.tag(Tag(key=tags.HttpStatus, val=resp.status_code))
span.tag(Tag(key=tags.HttpStatus, val=resp.status_code, overridable=True))
if resp.status_code >= 400:
span.error_occurred = True
return resp
Expand Down
2 changes: 1 addition & 1 deletion skywalking/plugins/sw_flask.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ def _sw_full_dispatch_request(this: Flask):
if resp.status_code >= 400:
span.error_occurred = True

span.tag(Tag(key=tags.HttpStatus, val=resp.status_code))
span.tag(Tag(key=tags.HttpStatus, val=resp.status_code, overridable=True))
return resp

def _sw_handle_user_exception(this: Flask, e):
Expand Down
51 changes: 46 additions & 5 deletions skywalking/plugins/sw_http_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ def _sw_handle(handler: BaseHTTPRequestHandler):

BaseHTTPRequestHandler.handle = _sw_handle

def _sw_send_response_only(self, code, *args, **kwargs):
self._status_code = code

return _send_response_only(self, code, *args, **kwargs)

_send_response_only = BaseHTTPRequestHandler.send_response_only
BaseHTTPRequestHandler.send_response_only = _sw_send_response_only


def wrap_werkzeug_request_handler(handler):
"""
Expand All @@ -53,15 +61,38 @@ def _wrap_run_wsgi():
carrier = Carrier()
for item in carrier:
item.val = handler.headers[item.key.capitalize()]
with context.new_entry_span(op=handler.path, carrier=carrier) as span:
path = handler.path or '/'
with context.new_entry_span(op=path.split("?")[0], carrier=carrier) as span:
url = 'http://' + handler.headers["Host"] + path if 'Host' in handler.headers else path
span.layer = Layer.Http
span.component = Component.General
span.peer = '%s:%s' % handler.client_address
span.tag(Tag(key=tags.HttpMethod, val=handler.command))
return _run_wsgi()
span.tag(Tag(key=tags.HttpUrl, val=url))
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid that this will break the tests (not sure though, didn't check the tests), you added a new tag, so you may need to adjust the expected data file to match it.

As you're actively contributing ❤️ , I hope you can set up the test environment locally so that you can quickly verify the part you modified (if you need any help, feel free to ping me anytime on Slack or GitHub), no need to wait for the GitHub Actions (GHA) for a long time, 'cause it runs all the tests

Copy link
Member

Choose a reason for hiding this comment

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

Basically, we only need docker (and docker-compose), and you can run one or multiple tests under https://github.com/apache/skywalking-python/tree/master/tests just like you run a unit test in Python, e.g. pytest -v tests/plugin/sw_http

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is what I meant with the previous PR that the tags changed, but in that case only failing cases would change, with this change normal tests will change so yes they may need to be updated to expect the new tag.

Maybe will look into setting up test env but don't have too much more to add, maybe an sw_wsgiref plugin but will see, so for now lets see how this test comes back and hope its an easy fix.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe will look into setting up test env but don't have too much more to add, maybe an sw_wsgiref plugin but will see, so for now lets see how this test comes back and hope its an easy fix.

OK, so the there are diffs in test results, if you don't want to set up the env locally, they should be helpful to adjust, or, if you need any help, I can debug locally for you

Copy link
Member

Choose a reason for hiding this comment

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

image

Just FYI, the circled is what need to be adjusted, and ignore the isError part, and something like gt 0 is for verifying uncertain integer (timestamp in this case)

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 looked into it and made the changes, this latest push should succeed. Also noticed why the previous PR didn't have problems, there are no tests for fail cases? 404? 500? There really should be...


try:
return _run_wsgi()
finally:
status_code = int(getattr(handler, '_status_code', -1))
if status_code > -1:
span.tag(Tag(key=tags.HttpStatus, val=status_code, overridable=True))
if status_code >= 400:
span.error_occurred = True

handler.run_wsgi = _wrap_run_wsgi

def _sw_send_response(self, code, *args, **kwargs):
self._status_code = code

return _send_response(self, code, *args, **kwargs)

WSGIRequestHandler = handler.__class__

if not getattr(WSGIRequestHandler, '_sw_wrapped', False):
_send_response = WSGIRequestHandler.send_response
WSGIRequestHandler.send_response = _sw_send_response
WSGIRequestHandler._sw_wrapped = True


def wrap_default_request_handler(handler):
http_methods = ('GET', 'HEAD', 'POST', 'PUT', 'DELETE', 'CONNECT', 'OPTIONS', 'TRACE', 'PATCH')
Expand All @@ -78,12 +109,22 @@ def _sw_do_method():
carrier = Carrier()
for item in carrier:
item.val = handler.headers[item.key.capitalize()]
with context.new_entry_span(op=handler.path, carrier=carrier) as span:
path = handler.path or '/'
with context.new_entry_span(op=path.split("?")[0], carrier=carrier) as span:
url = 'http://' + handler.headers["Host"] + path if 'Host' in handler.headers else path
span.layer = Layer.Http
span.component = Component.General
span.peer = '%s:%s' % handler.client_address
span.tag(Tag(key=tags.HttpMethod, val=method))

_do_method()
span.tag(Tag(key=tags.HttpUrl, val=url))

try:
_do_method()
finally:
status_code = int(getattr(handler, '_status_code', -1))
if status_code > -1:
span.tag(Tag(key=tags.HttpStatus, val=status_code, overridable=True))
if status_code >= 400:
span.error_occurred = True

setattr(handler, 'do_' + method, _sw_do_method)
2 changes: 1 addition & 1 deletion skywalking/plugins/sw_requests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ def _sw_request(this: Session, method, url,
proxies,
hooks, stream, verify, cert, json)

span.tag(Tag(key=tags.HttpStatus, val=res.status_code))
span.tag(Tag(key=tags.HttpStatus, val=res.status_code, overridable=True))
if res.status_code >= 400:
span.error_occurred = True

Expand Down
4 changes: 2 additions & 2 deletions skywalking/plugins/sw_tornado.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ async def _sw_get_response(self, *args, **kwargs):
span.tag(
Tag(key=tags.HttpUrl, val='{}://{}{}'.format(request.protocol, request.host, request.path)))
result = old_execute(self, *args, **kwargs)
span.tag(Tag(key=tags.HttpStatus, val=self._status_code))
span.tag(Tag(key=tags.HttpStatus, val=self._status_code, overridable=True))
if isawaitable(result):
result = await result
if self._status_code >= 400:
Expand All @@ -90,7 +90,7 @@ def _sw_get_response(self, *args, **kwargs):
span.tag(
Tag(key=tags.HttpUrl, val='{}://{}{}'.format(request.protocol, request.host, request.path)))
result = yield from old_execute(self, *args, **kwargs)
span.tag(Tag(key=tags.HttpStatus, val=self._status_code))
span.tag(Tag(key=tags.HttpStatus, val=self._status_code, overridable=True))
if self._status_code >= 400:
span.error_occurred = True
return result
Expand Down
2 changes: 1 addition & 1 deletion skywalking/plugins/sw_urllib3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def _sw_request(this: RequestMethods, method, url, fields=None, headers=None, **

res = _request(this, method, url, fields=fields, headers=headers, **urlopen_kw)

span.tag(Tag(key=tags.HttpStatus, val=res.status))
span.tag(Tag(key=tags.HttpStatus, val=res.status, overridable=True))
if res.status >= 400:
span.error_occurred = True

Expand Down
4 changes: 2 additions & 2 deletions skywalking/plugins/sw_urllib_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,13 @@ def _sw_open(this: OpenerDirector, fullurl, data, timeout):
try:
res = _open(this, fullurl, data, timeout)
except HTTPError as e:
span.tag(Tag(key=tags.HttpStatus, val=e.code))
span.tag(Tag(key=tags.HttpStatus, val=e.code, overridable=True))
raise
finally: # we do this here because it may change in _open()
span.tag(Tag(key=tags.HttpMethod, val=fullurl.get_method()))
span.tag(Tag(key=tags.HttpUrl, val=fullurl.full_url))

span.tag(Tag(key=tags.HttpStatus, val=res.code))
span.tag(Tag(key=tags.HttpStatus, val=res.code, overridable=True))
if res.code >= 400:
span.error_occurred = True

Expand Down
13 changes: 6 additions & 7 deletions skywalking/trace/span.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,13 @@ def log(self, ex: Exception) -> 'Span':
return self

def tag(self, tag: Tag) -> 'Span':
if not tag.overridable:
self.tags.append(deepcopy(tag))
return self
if tag.overridable:
for i, t in enumerate(self.tags):
if t.key == tag.key:
self.tags[i] = deepcopy(tag)
return self

for t in self.tags:
if t.key == tag.key:
t.val = tag.val
break
self.tags.append(deepcopy(tag))

return self

Expand Down