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.
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
[Plugin] http servers set URL and status tags #85
Conversation
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)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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...
This last commit is the following - Fixed sw_urllib_request.py:sw_open() - Added default values to parameters because without them it would fail when called internally in urllib.request without a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much
data
argument, like inHTTPRedirectHandler.http_error_302()
.