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/12/11 19:31:16 UTC
[GitHub] [skywalking-python] tom-pytel opened a new pull request #100: [WIP] started on aiohttp
tom-pytel opened a new pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100
Started on new plugin for aiohttp, requester works but still need to add server component. Again hoping you can handle the tests for this. Will see about adding component ID and maybe logo myself when it is done.
<!-- 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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541569017
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
Review comment:
We need to review these plugins and if they really did store username/password, we need to remove them explicitly, usually as the name `peer` indicates, we only store host/IP and port into it
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541557312
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
Review comment:
> > The Java agent doesn't record this
>
>
>
> So just Python and Node, both incoming and outgoing, http and any other protocols (layers)?
Do you mean peer or password? We usually only record host/ip and port in `peer`
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541567859
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
Review comment:
Like for example in sw_requests.py, the following code will store the user and password if I am not mistaken:
```py
def _sw_request(this: Session, method, url,
params=None, data=None, headers=None, cookies=None, files=None,
auth=None, timeout=None, allow_redirects=True, proxies=None,
hooks=None, stream=None, verify=None, cert=None, json=None):
from urllib.parse import urlparse
url_param = urlparse(url)
...
context = get_context()
with context.new_exit_span(op=url_param.path or "/", peer=url_param.netloc) as span:
```
I thinkthis is repeated across several plugins.
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541567859
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
Review comment:
Like for example in sw_requests.py, the following code will store the user and password if I am not mistaken:
```py
def _sw_request(this: Session, method, url,
params=None, data=None, headers=None, cookies=None, files=None,
auth=None, timeout=None, allow_redirects=True, proxies=None,
hooks=None, stream=None, verify=None, cert=None, json=None):
from urllib.parse import urlparse
url_param = urlparse(url)
...
context = get_context()
with context.new_exit_span(op=url_param.path or "/", peer=url_param.netloc) as span:
```
I think this is repeated across several plugins.
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#issuecomment-743385592
Here is an async test snipped:
```py
import asyncio
import aiohttp
from skywalking import agent
from skywalking.decorators import trace
from skywalking.trace.context import get_context
import skywalking.trace.context as context
cities = ['paris', 'lyon', 'nice']
agent.start()
async def child(i):
async with aiohttp.ClientSession() as session:
url = f"https://geo.api.gouv.fr/communes?nom={cities[i]}&fields=nom,region&format=json&geometry=centr"
async with session.get(url) as response:
return await response.json()
loop = asyncio.get_event_loop()
with get_context().new_local_span('/parent'):
loop.run_until_complete(
asyncio.gather(
*(trace(f'child{i}')(child)(i) for i in range(3))
)
)
```
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541554607
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
Review comment:
Yes its probably better not to. The thing is I copied this behavior from the other plugins so they will need to be changed to explicitly remove the password as well since `urllib.parse` always returns it as part of `netloc` if it is present and so would be recorded. Then the Node agent also, as well I am wondering if the Java plugins record this?
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541421088
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
+ (url.host or '') + \
+ (':' + str(url.explicit_port) if url.explicit_port is not None else '')
+ context = get_context()
+
+ with context.new_exit_span(op=url.path or "/", peer=peer) as span:
+ span.layer = Layer.Http
+ span.component = Component.Unknown # TODO: add Component.aiohttp
+ span.tag(Tag(key=tags.HttpMethod, val=method.upper()))
+ span.tag(Tag(key=tags.HttpUrl, val=url))
Review comment:
The weird thing is the same code doesn't trigger warning in the other plugins.
----------------------------------------------------------------
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] TomMD commented on a change in pull request #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
TomMD commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541206047
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
+ (url.host or '') + \
+ (':' + str(url.explicit_port) if url.explicit_port is not None else '')
+ context = get_context()
+
+ with context.new_exit_span(op=url.path or "/", peer=peer) as span:
+ span.layer = Layer.Http
+ span.component = Component.Unknown # TODO: add Component.aiohttp
+ span.tag(Tag(key=tags.HttpMethod, val=method.upper()))
+ span.tag(Tag(key=tags.HttpUrl, val=url))
Review comment:
Congratulations on the 100th pull request for skywalking-python!
I think this is the second "missing argument" error on Tag.__init__. If you'd like I can make a PR (or you are free to) that adds `igore = [ "Missing argument" ]` to the config.toml. Otherwise I'm likely to do something in the backend that removes this seemingly common seemingly false positive.
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541555287
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
+ (url.host or '') + \
+ (':' + str(url.explicit_port) if url.explicit_port is not None else '')
+ context = get_context()
+
+ with context.new_exit_span(op=url.path or "/", peer=peer) as span:
+ span.layer = Layer.Http
+ span.component = Component.Unknown # TODO: add Component.aiohttp
+ span.tag(Tag(key=tags.HttpMethod, val=method.upper())) # pyre-ignore
+ span.tag(Tag(key=tags.HttpUrl, val=url)) # pyre-ignore
+
+ carrier = span.inject()
+ headers = kwargs.get('headers')
+
+ if headers is None:
+ headers = CIMultiDict()
+ elif not isinstance(headers, (MultiDictProxy, MultiDict)):
+ headers = CIMultiDict(headers)
Review comment:
Put `headers` back to `kwargs`, yes I meant to do this but forgot, this is why reviews are good :smiley:
As for "overriding", as far as I can see it should be ok since `aiohttp` doesn't do anything with the headers until it executes the following in `ClientSession._prepare_headers()`:
```py
if not isinstance(headers, (MultiDictProxy, MultiDict)):
headers = CIMultiDict(headers)
```
Which is actually where I got it from, and yes `CIMultiDict` is an instance of `MultiDict` so I am just executing what would be done anyway a little earlier.
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541555720
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
Review comment:
> Yes its probably better not to. The thing is I copied this behavior from the other plugins so they will need to be changed to explicitly remove the password as well since `urllib.parse` always returns it as part of `netloc` if it is present and so would be recorded. Then the Node agent also, as well I am wondering if the Java plugins record this?
The Java agent doesn't record this
----------------------------------------------------------------
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 closed pull request #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
kezhenxu94 closed pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541470799
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
+ (url.host or '') + \
+ (':' + str(url.explicit_port) if url.explicit_port is not None else '')
+ context = get_context()
+
+ with context.new_exit_span(op=url.path or "/", peer=peer) as span:
+ span.layer = Layer.Http
+ span.component = Component.Unknown # TODO: add Component.aiohttp
+ span.tag(Tag(key=tags.HttpMethod, val=method.upper())) # pyre-ignore
+ span.tag(Tag(key=tags.HttpUrl, val=url)) # pyre-ignore
+
+ carrier = span.inject()
+ headers = kwargs.get('headers')
+
+ if headers is None:
+ headers = CIMultiDict()
+ elif not isinstance(headers, (MultiDictProxy, MultiDict)):
+ headers = CIMultiDict(headers)
Review comment:
Should put `headers` back to `kwargs`? Also, is it safe to "override" the existing headers if it's not `MultiDic(Proxy)`?
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
Review comment:
I don’t think we want to include `password` here
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541556097
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
Review comment:
> The Java agent doesn't record this
So just Python and Node, both incoming and outgoing, http and any other protocols (layers)?
----------------------------------------------------------------
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] muse-dev[bot] commented on a change in pull request #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
muse-dev[bot] commented on a change in pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#discussion_r541201647
##########
File path: skywalking/plugins/sw_aiohttp.py
##########
@@ -0,0 +1,65 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+from skywalking import Layer, Component
+from skywalking.trace import tags
+from skywalking.trace.context import get_context
+from skywalking.trace.tags import Tag
+
+
+def install():
+ from aiohttp import ClientSession
+ from multidict import CIMultiDict, MultiDict, MultiDictProxy
+ from yarl import URL
+
+ async def _sw_request(self: ClientSession, method: str, str_or_url, **kwargs):
+ url = URL(str_or_url)
+ peer = \
+ (url.scheme + '://' if url.scheme else '') + \
+ ((url.user or '') + ':' + (url.password or '') + '@' if url.user or url.password else '') + \
+ (url.host or '') + \
+ (':' + str(url.explicit_port) if url.explicit_port is not None else '')
+ context = get_context()
+
+ with context.new_exit_span(op=url.path or "/", peer=peer) as span:
+ span.layer = Layer.Http
+ span.component = Component.Unknown # TODO: add Component.aiohttp
+ span.tag(Tag(key=tags.HttpMethod, val=method.upper()))
+ span.tag(Tag(key=tags.HttpUrl, val=url))
Review comment:
*Missing argument:* Call `Tag.__init__` expects argument `overridable`.
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#issuecomment-743748244
Test snippet:
```py
from aiohttp import web
from skywalking import agent
agent.start()
async def handle(request):
name = request.match_info.get('name', "Anonymous")
text = "Hello, " + name
return web.Response(text=text)
app = web.Application()
app.add_routes([web.get('/', handle),
web.get('/{name}', handle)])
if __name__ == '__main__':
web.run_app(app, port=8000)
```
Am I missing anything to be stored or handled?
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#issuecomment-743772919
Hi @tom-pytel , sorry for closing this by mistake because of a force push, can you please reopen another PR?
----------------------------------------------------------------
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 edited a comment on pull request #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#issuecomment-743385592
Here is a very async test snippet:
```py
import asyncio
import aiohttp
from skywalking import agent
from skywalking.decorators import trace
from skywalking.trace.context import get_context
import skywalking.trace.context as context
cities = ['paris', 'lyon', 'nice']
agent.start()
async def child(i):
async with aiohttp.ClientSession() as session:
url = f"https://geo.api.gouv.fr/communes?nom={cities[i]}&fields=nom,region&format=json&geometry=centr"
async with session.get(url) as response:
return await response.json()
loop = asyncio.get_event_loop()
with get_context().new_local_span('/parent'):
loop.run_until_complete(
asyncio.gather(
*(trace(f'child{i}')(child)(i) for i in range(3))
)
)
```
----------------------------------------------------------------
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 #100: [WIP] started on aiohttp
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #100:
URL: https://github.com/apache/skywalking-python/pull/100#issuecomment-743751297
> Test snippet:
>
> ```python
> from aiohttp import web
> from skywalking import agent
>
> agent.start()
>
> async def handle(request):
> name = request.match_info.get('name', "Anonymous")
> text = "Hello, " + name
> return web.Response(text=text)
>
> app = web.Application()
> app.add_routes([web.get('/', handle),
> web.get('/{name}', handle)])
>
> if __name__ == '__main__':
> web.run_app(app, port=8000)
> ```
>
> Am I missing anything to be stored or handled?
I’ll test locally, also, can you please uncomment the PR template so that we can check the items one by one (some may be checked by me though)
----------------------------------------------------------------
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