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.

Sign up for GitHub

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

Jump to bottom

fix(py): fix redis connection #617

Merged
merged 3 commits into from
May 22, 2024
Merged

fix(py): fix redis connection #617

merged 3 commits into from
May 22, 2024

Conversation

eeliu
Copy link
Collaborator

@eeliu eeliu commented May 22, 2024 •
edited
Loading

Describe your changes

  • use format_host

Issue ticket number and link

#612

Checklist before requesting a review

  • I have performed a self-review of my code
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics?
  • Will this be part of a product update? If yes, please write one phrase about this update.

@eeliu eeliu mentioned this pull request May 22, 2024
Closed
@eeliu eeliu added the bug Something isn't working label May 22, 2024
@eeliu eeliu force-pushed the fix-612-format_host branch from c3cf30f to b4575e9 Compare May 22, 2024 05:55
@codecov Codecov
Copy link

codecov bot commented May 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.67%. Comparing base (c93e207) to head (ee1a943).
Report is 14 commits behind head on dev.

Additional details and impacted files 
@@            Coverage Diff             @@
##              dev     #617      +/-   ##
==========================================
- Coverage   89.61%   80.67%   -8.95%     
==========================================
  Files          22       22              
  Lines        1569     1521      -48     
  Branches        0      166     +166     
==========================================
- Hits         1406     1227     -179     
+ Misses        163      162       -1     
- Partials        0      132     +132

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

eeliu
eeliu commented May 22, 2024
View reviewed changes
plugins/PY/pinpointPy/libs/_pyRedis/PyRedisPlugins.py
@@ -26,6 +26,10 @@
from pinpointPy import Defines


def format_host(host, port, db) -> str:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as the destination format must be the same in a plugin.

quicksandznzn
quicksandznzn reviewed May 22, 2024
View reviewed changes
plugins/PY/pinpointPy/libs/_pyRedis/PyRedisPlugins.py
@@ -62,8 +68,9 @@ def onBefore(self, parentId, *args, **kwargs):
pipeLine = args[0]
# fixed: Redis Collections Not release #612
# @quicksandznzn
connection_kwargs = pipeLine.connection_pool.connection_kwargs
pinpoint.add_trace_header(
Copy link

Choose a reason for hiding this comment

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

must default value
like this
pinpoint.add_trace_header( Defines.PP_DESTINATION, format_host(connection_kwargs.get('host','localhost'), connection_kwargs.get('port',6379), connection_kwargs.get('db',0)) , trace_id)

Copy link

Choose a reason for hiding this comment

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

@eeliu

Copy link

Choose a reason for hiding this comment

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

I have a questions, this dst value look at only show in apm web ,the java code call redis show this , python code why need host port db
image

Copy link

Choose a reason for hiding this comment

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

If you don't mind. please add my wechat account Quicksand_ZN

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a questions, this dst value look at only show in apm web ,the java code call redis show this , python code why need host port db image

Good question,

it's ok to keep the same format, but sometime user needs more, like port, db( mostly on redis cluster)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

connection_kwargs

I will add them.

@quicksandznzn
Copy link

LGTM

@eeliu eeliu merged commit eb4b891 into pinpoint-apm:dev May 22, 2024
17 of 18 checks passed
@eeliu eeliu deleted the fix-612-format_host branch May 22, 2024 07:30
eeliu added a commit to eeliu/pinpoint-c-agent that referenced this pull request Jul 9, 2024
@eeliu @mayingping-Bella
@dependabot @Evelyn0927
Ci upload assert (#66)
c05095f 
* support windows python (pinpoint-apm#592)

* feat(id): support unique id

- unique id in ms
- refactor common library
- remove dependence from shm
- php-agent 0.5.0
- python-agent v1.2.1

* feat(win32): support win32

- python-agent
- common library
- refactor common library
- fix bug when pooling
- cibuildwheel
- code style
- fix bug in endTrace

* fix bug in thread mode

* remove ambiguous API

* pipy 1.2.4 release test

* test char encoding

* fix bug in python

* hello packet, avoiding conflict

* fix bug in flask

* fix convert problem

* delete 'import MySQLdb' (pinpoint-apm#593)

Co-authored-by: eeliu <27064129+eeliu@users.noreply.github.com>

* fix wheels bug

* intro docs

* feat:grpc-python (pinpoint-apm#588)

- api and example

* add docs for grpc

* Bump pymongo from 4.6.0 to 4.6.3 in /plugins/PY (pinpoint-apm#594)

Bumps [pymongo](https://github.com/mongodb/mongo-python-driver) from 4.6.0 to 4.6.3.
- [Release notes](https://github.com/mongodb/mongo-python-driver/releases)
- [Changelog](https://github.com/mongodb/mongo-python-driver/blob/master/doc/changelog.rst)
- [Commits](mongodb/mongo-python-driver@4.6.0...4.6.3)

---
updated-dependencies:
- dependency-name: pymongo
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* feat(agent): sql v3 support (pinpoint-apm#595)

* feat(agent): sql v3 support

- remove metadata.go

* fix build script

* modify docs

* feat(py): PostgreSql (pinpoint-apm#599)

- psycopg2
- version :1.2.1

* fix import error

> PsycopgPlugins should follow the psycopg2

* feat(php): simple-php (pinpoint-apm#600)

- example for php

> hopes to help  pinpoint-apm#596

* fix(agent): status_code (pinpoint-apm#603)

- treats >=400  as an error

>pinpoint-apm#602

* correct the message format

* support collector-agent on Arm (pinpoint-apm#605)

* add arm

> pinpoint-apm#604

* docs(py): Guide for exclude url (pinpoint-apm#610)

> user path route

pinpoint-apm#609

* fix(py): redis plugin (pinpoint-apm#613)

- fix: Redis Collections Not release pinpoint-apm#612

* fix ci failed

- docs: python plugins

* --- (pinpoint-apm#616)

updated-dependencies:
- dependency-name: pymysql
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* --- (pinpoint-apm#615)

updated-dependencies:
- dependency-name: requests
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix(py): fix redis connection (pinpoint-apm#617)

- use format_host
* add default value

* update test pinpointpy 1.3.4

* feat(collector-agent): error (pinpoint-apm#608)

- error analysis
- common,collector-agent  `EXP_V2`
- python new api `add_exception`

* Bump fastapi from 0.104.1 to 0.109.1 in /plugins/PY (pinpoint-apm#614)

Bumps [fastapi](https://github.com/tiangolo/fastapi) from 0.104.1 to 0.109.1.
- [Release notes](https://github.com/tiangolo/fastapi/releases)
- [Commits](fastapi/fastapi@0.104.1...0.109.1)

---
updated-dependencies:
- dependency-name: fastapi
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: eeliu <27064129+eeliu@users.noreply.github.com>

* --- (pinpoint-apm#618)

updated-dependencies:
- dependency-name: requests
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: eeliu <27064129+eeliu@users.noreply.github.com>

* Bump pymysql from 1.1.0 to 1.1.1 in /testapps/PY (pinpoint-apm#619)

Bumps [pymysql](https://github.com/PyMySQL/PyMySQL) from 1.1.0 to 1.1.1.
- [Release notes](https://github.com/PyMySQL/PyMySQL/releases)
- [Changelog](https://github.com/PyMySQL/PyMySQL/blob/main/CHANGELOG.md)
- [Commits](PyMySQL/PyMySQL@v1.1.0...v1.1.1)

---
updated-dependencies:
- dependency-name: pymysql
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: eeliu <27064129+eeliu@users.noreply.github.com>

* build(go): go install (pinpoint-apm#622)

- support `go install`

* update for collector-agent

* fix(py): unwanted trace_id (pinpoint-apm#627)

- fix collector-agent compiled problem
- patch for pinpoint-apm#626

* support c level plugin in php (pinpoint-apm#623)

* test c-level plugins

* test pinpoint_join_cut

* use zend_string_release

- fix bug in pinpoint_interceptor_handler_entry

* add try_end

* test pinpoint_join_cut

* test pdo

* ci(versions): test php versions (#63)

- covered all phpt by docker compose
- pdo,redis,curl
- fix CI

* remote php5 support

* support ci for php7

* add php8.0 testCI

* add php8.3

* rename internal API

- pinpoint_start_trace => _pinpoint_start_trace

* update pinpoint_php module version

* add testcase for simplephp

* update docs

* Feat join cut get caller (#64)

* feat(php): pinpoint_get_caller_arg
* pinpoint_get_caller_arg and phpt
* update pinpoint_php module version 0.5.2
* - remove `fastapi==0.109.1` , it crashes CI
* simple_php for curl/redis/memcache/mysql

* test pack_php_module

* test wordpress

* enable logger

* test phpmyadmin

* fix phpmyadmin

* test(php): more popular project (#65)

- php wordpress, phpmyadmin
- java_call_app

* fix simplephp

* for wilderness test

* change package naming

* update docs

* fix extends bug in pinpoint-php

* support flarum

* support cachethq

* update docs

* update CN docs

* PHP DOC KR version update (pinpoint-apm#638)

* update docs

* update script for pinpoint_php (pinpoint-apm#639)

* fix version problem

* fix(php): yii2

- fix pinpoint-apm#640 yii2 plugins

* fix docs

* script for automatically uploading assert

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: mayingping-Bella <72844069+mayingping-Bella@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Evelyn0927 <104180702+Evelyn0927@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quicksandznzn quicksandznzn quicksandznzn left review comments

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eeliu @quicksandznzn