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.

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] add falcon plugin #146

Merged
merged 20 commits into from
Aug 11, 2021
Merged

[plugin] add falcon plugin #146

merged 20 commits into from
Aug 11, 2021

Conversation

probeyang
Copy link
Contributor

image

iprobeyang and others added 7 commits April 27, 2021 15:49
add tornado5+ and tornado6+ support
add tornado5.1.1 test case and format tornado plugin code
add the plugin of falcon
format code
expected data change, falcon component code change to 7012.
@kezhenxu94 kezhenxu94 added the plugin Plugin label Aug 6, 2021
@kezhenxu94 kezhenxu94 added this to the 0.7.0 milestone Aug 6, 2021
@tom-pytel
Copy link
Contributor

New sw_tornado5.py in this PR is identical to the old sw_tornado.py, so why split it into two different plugins?

Also it is based on an older version in master which is missing a peer address fix and the recent exclude by http method functionality.

@@ -43,6 +43,7 @@ class Component(Enum):
Pyramid = 7009
Psycopg = 7010
Celery = 7011
Falcon = 7012
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i add it.

@tom-pytel
Copy link
Contributor

tom-pytel commented Aug 7, 2021

I am good with this (pending last @kezhenxu94 issue main repo updates of course).

Copy link
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.

Look good to me, please add component id and logo to main repo and UI repo

@tom-pytel
Copy link
Contributor

Look good to me, please add component id and logo to main repo and UI repo

Not quite good yet, need to update for new Tag change.

@tom-pytel tom-pytel self-requested a review August 8, 2021 13:37
req = request.Request(env, RequestOptions())
span.op = str(req.url).split("?")[0]
span.peer = "%s:%s" % (req.remote_addr, req.port)
span.tag(Tag(key=tags.HttpMethod, val=req.method))
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be changed to:

            span.tag(TagHttpMethod(req.method))
            span.tag(TagHttpURL(str(req.url)))
            if req.params:
                span.tag(TagHttpParams(params_tostring(req.params)[0:]))

And the import at the top to:

from skywalking.trace.tags import TagHttpMethod, TagHttpURL, TagHttpParams

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, status tag not being set?

Copy link
Member

Choose a reason for hiding this comment

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

Also, status tag not being set?

@probeyang can you add the status tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks a lot, i found my code is older than apache/skywalking-python, i upgrade it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i will set status argument

Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

Document and changelog are not updated.

@probeyang
Copy link
Contributor Author

Document and changelog are not updated.

ok , i will update it.

update changelog and plugins.md.
Copy link
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.

I think you should not update the submodule protocol?

delete unused tags
Add New Argument: TagHttpStatusCode Span Tag
@kezhenxu94
Copy link
Member

You need to revert the submodule update protocol, others look good

@probeyang
Copy link
Contributor Author

You need to revert the submodule update protocol, others look good

ok

revert protocol
revert protocol
Update expected.data.yml
@kezhenxu94 kezhenxu94 requested a review from tom-pytel August 11, 2021 06:29
@kezhenxu94 kezhenxu94 merged commit e118f26 into apache:master Aug 11, 2021
Superskyyy added a commit to Superskyyy/skywalking-python that referenced this pull request Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin Plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants