Skip to content

[Plugin] moved Method & Url tags before requests#84

Merged
kezhenxu94 merged 2 commits intoapache:masterfrom
tom-pytel:master
Nov 20, 2020
Merged

[Plugin] moved Method & Url tags before requests#84
kezhenxu94 merged 2 commits intoapache:masterfrom
tom-pytel:master

Conversation

@tom-pytel
Copy link
Copy Markdown
Contributor

  • 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.

@tom-pytel
Copy link
Copy Markdown
Contributor Author

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?

@kezhenxu94
Copy link
Copy Markdown
Member

kezhenxu94 commented Nov 20, 2020

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

@kezhenxu94 kezhenxu94 added enhancement New feature or request plugin Plugin labels Nov 20, 2020
@kezhenxu94 kezhenxu94 added this to the 0.5.0 milestone Nov 20, 2020
@tom-pytel
Copy link
Copy Markdown
Contributor Author

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.

Comment thread skywalking/plugins/sw_urllib_request.py
Comment thread skywalking/plugins/sw_urllib_request.py
Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

Thanks

@kezhenxu94 kezhenxu94 merged commit 5076aa3 into apache:master Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request plugin Plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants