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 2021/06/05 13:25:59 UTC
[GitHub] [skywalking-python] tom-pytel opened a new pull request #122: WIP initial psycopg plugin
tom-pytel opened a new pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122
<!-- 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`
-->
I will do the other plugin stuff once the test is stable.
--
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 #122: WIP initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122#issuecomment-855284579
Hey @kezhenxu94, I have no idea why the test is failing. I copied the framework from pymsql test which works and the code in `provider.py` works by itself but for some reason running as a test it is getting:
```
Exception: Wait time exceeded 100 sec. Exception HTTPConnectionPool(host='0.0.0.0', port=9090): Max retries exceeded with url: /users (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fc586e6b040>: Failed to establish a new connection: [Errno 111] Connection refused'))
```
--
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 #122: WIP initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
tom-pytel edited a comment on pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122#issuecomment-855406558
Ok, one thing to resolve before merge. I reused `config.mysql_trace_sql_parameters` and `config.mysql_sql_parameters_max_length` options in this plugin just to get it running but the final param should be one of two cases:
1. Add separate `config.postgre_trace_sql_parameters` and `config.postgre_sql_parameters_max_length`.
2. Convert `config.mysql_trace_sql_parameters` to just `config.trace_sql_parameters` as well as the length option and have all SQL agents share this to avoid explosion of options as different SQL agents are added (since odds are you will not be using mysql and postgresql simultaneously in the same project) - this is my preferred option.
And another idea unrelated to which option above is taken. If you are not locked into the current options scheme - reduce the two parameters `config.mysql_trace_sql_parameters` and `config.mysql_sql_parameters_max_length` into a single option `config.mysql_parameters` (or something like that) which gives the length and if length 0 is specified (default) then disables parameters?
--
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 #122: WIP initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122#discussion_r646058482
##########
File path: tests/plugin/sw_psycopg2/docker-compose.yml
##########
@@ -0,0 +1,79 @@
+#
+# 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.
+#
+
+version: '2.1'
+
+services:
+ collector:
+ extends:
+ service: collector
+ file: ../docker/docker-compose.base.yml
+
+ postgres:
+ image: postgres:11
+ hostname: postgres
+ ports:
+ - 5432:5432
+ environment:
+ - POSTGRES_USER=root
+ - POSTGRES_PASSWORD=root
+ - POSTGRES_DB=admin
+ healthcheck:
+ test: ["CMD", "bash", "-c", "cat < /dev/null > /dev/tcp/127.0.0.1/3306"]
Review comment:
Hi, this is the reason why tests failed, the exposed port is 5432 but this checks 3306, I believe it's a copy-paste typo.
<img width="1880" alt="Screen Shot 2021-06-06 at 09 42 31" src="https://user-images.githubusercontent.com/15965696/120909811-8d769c00-c6ab-11eb-9b6d-bc34a12a7421.png">
```suggestion
test: ["CMD", "bash", "-c", "cat < /dev/null > /dev/tcp/127.0.0.1/5432"]
```
--
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 #122: WIP initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122#issuecomment-855406558
Ok, one thing to resolve before merge. I reused `config.mysql_trace_sql_parameters` and `config.mysql_sql_parameters_max_length` options in this plugin just to get it running but the final param should be one of two cases:
1. Add separate `config.postgre_trace_sql_parameters` and `config.postgre_sql_parameters_max_length`.
2. Convert `config.mysql_trace_sql_parameters` to just `config.trace_sql_parameters` and have all SQL agents share this to avoid explosion of options as different SQL agents are added (since odds are you will not be using mysql and postgresql simultaneously in the same project) - this is my preferred option.
And another idea unrelated to which option above is taken. If you are not locked into the current options scheme - reduce the two parameters `config.mysql_trace_sql_parameters` and `config.mysql_sql_parameters_max_length` into a single option `config.mysql_parameters` (or something like that) which gives the length and if length 0 is specified (default) then disables parameters?
--
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 #122: WIP initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
tom-pytel commented on a change in pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122#discussion_r646144027
##########
File path: skywalking/__init__.py
##########
@@ -40,6 +40,7 @@ class Component(Enum):
Sanic = 7007
AioHttp = 7008
Pyramid = 7009
+ Psycopg = 7010
Review comment:
I've got that done just was waiting for this part to finalize.
--
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 merged pull request #122: Initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
tom-pytel merged pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122
--
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] wu-sheng commented on pull request #122: WIP initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122#issuecomment-855248387
@tom-pytel Take a look at slack. I left a message for your Apache committer to further process. It is in your hands to complete them.
--
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 #122: WIP initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 edited a comment on pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122#issuecomment-855407509
> And another idea unrelated to which option above is taken. If you are not locked into the current options scheme - reduce the two parameters `config.mysql_trace_sql_parameters` and `config.mysql_sql_parameters_max_length` into a single option `config.mysql_parameters` (or something like that) which gives the length and if length 0 is specified (default) then disables parameters?
This is an optimization I've been thinking about, I'm +1 to it.
Also, I prefer (2.), since there is rare situation that a service will use multiple DB layers and not use the same configurations for them.
So the final config will be `config.sql_parameters_max_length`, or similar thing, please make sure to update the doc as well
--
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 #122: WIP initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122#discussion_r646142831
##########
File path: skywalking/__init__.py
##########
@@ -40,6 +40,7 @@ class Component(Enum):
Sanic = 7007
AioHttp = 7008
Pyramid = 7009
+ Psycopg = 7010
Review comment:
Remember to add this in main repo and UI repo
--
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 #122: WIP initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
muse-dev[bot] commented on a change in pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122#discussion_r645991625
##########
File path: tests/plugin/sw_psycopg2/services/provider.py
##########
@@ -0,0 +1,46 @@
+#
Review comment:
*Parsing failure:* Could not parse file at tests/plugin/sw_psycopg2/services/provider.py:38:38-38:38
(at-me [in a reply](https://docs.muse.dev/docs/talk-to-muse/) with `help` or `ignore`)
--
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 #122: WIP initial psycopg plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #122:
URL: https://github.com/apache/skywalking-python/pull/122#issuecomment-855407509
> And another idea unrelated to which option above is taken. If you are not locked into the current options scheme - reduce the two parameters `config.mysql_trace_sql_parameters` and `config.mysql_sql_parameters_max_length` into a single option `config.mysql_parameters` (or something like that) which gives the length and if length 0 is specified (default) then disables parameters?
This is an optimization I've been thinking about, I'm +1 to it.
Also, I prefer (2.), since there is rare situation that a service will use multiple DB layers and not use the same configurations for them.
--
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