You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/11/19 18:15:13 UTC
[GitHub] [skywalking-python] tom-pytel opened a new pull request #84: [Plugin] moved Method & Url tags before requests
tom-pytel opened a new pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84
* Moved set of HttpMethod and HttpUrl tags to before requests to make sure they are recorded even if request fails.
* Specifically catching HTTPError in `sw_urllib_request.py` to properly set HttpStatus in this case.
<!-- Uncomment the following checklist WHEN AND ONLY WHEN you're adding a new plugin -->
<!--
- [ ] Add a test case for the new plugin
- [ ] Add a component id in [the main repo](https://github.com/apache/skywalking/blob/master/oap-server/server-bootstrap/src/main/resources/component-libraries.yml#L415)
- [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)
- [ ] Rebuild the `requirements.txt` by running `tools/env/build_requirements_(linux|windows).sh`
-->
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on a change in pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#discussion_r527348094
##########
File path: skywalking/plugins/sw_urllib_request.py
##########
@@ -38,15 +39,22 @@ 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)
Review comment:
I think this breaks the tests, `fullurl.get_method()` is not static, it differs before/after `_open(this, fullurl, data, timeout)`.
Maybe `_open` set some internal states that `get_method()` will leverages?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] tom-pytel commented on pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#issuecomment-730752688
> > any way to see if these test failures are that or something else?
>
> Here it is https://github.com/apache/skywalking-python/pull/84/checks?check_run_id=1426036106
>
> Pay attention to the diff in the log
Yes I did see that but I am not familiar with the test so I don't know if the diff means that HttpMethod and HttpUrl tags appeared where they were not before or if it means some other failure mode.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] tom-pytel commented on a change in pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#discussion_r527680270
##########
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...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 edited a comment on pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#issuecomment-730742813
> any way to see if these test failures are that or something else?
Here it is https://github.com/apache/skywalking-python/pull/84/checks?check_run_id=1426036106
Pay attention to the diff in the log
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on a change in pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#discussion_r527675878
##########
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:
Please note that tag with the same key can co-exist, here you tag the span twice with the same key (`tags.HttpMethod`), which breaks the tests
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] tom-pytel commented on a change in pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#discussion_r527708094
##########
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:
Well, because this does change behavior. If there are tests that previously did not expect these tags due to testing a fail case and they are now returned then that will be a difference.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on a change in pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#discussion_r527716939
##########
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:
> Well, because this does change behavior. If there are tests that previously did not expect these tags due to testing a fail case and they are now returned then that will be a difference.
OK, that makes sense, IIRC, we didn't test fail case, so the tests should pass
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#issuecomment-730742813
> any way to see if these test failures are that or something else?
Here it is https://github.com/apache/skywalking-python/pull/84/checks?check_run_id=1426036106
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] tom-pytel commented on a change in pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#discussion_r527680270
##########
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, lets see...
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on a change in pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#discussion_r527348094
##########
File path: skywalking/plugins/sw_urllib_request.py
##########
@@ -38,15 +39,22 @@ 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)
Review comment:
I think this breaks the tests, `fullurl.get_method()` is not static, it differs before/after `_open(this, fullurl, data, timeout)`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 merged pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
kezhenxu94 merged pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [skywalking-python] kezhenxu94 commented on a change in pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [skywalking-python] tom-pytel commented on pull request #84: [Plugin] moved Method & Url tags before requests
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #84:
URL: https://github.com/apache/skywalking-python/pull/84#issuecomment-730578719
I expect these changes to change some test results, adding HttpMethod and HttpUrl tags in places where they may not have been before and changing some endpoint names (removing args following ?), any way to see if these test failures are that or something else?
----------------------------------------------------------------
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:
users@infra.apache.org