-
Notifications
You must be signed in to change notification settings - Fork 35
Fix: RabbitMQ check proper span type #309
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
Conversation
manojpandey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdimitra thanks for working on this - I added some comments in the review :D
Good work!
| self.k = 3 # intermediate span | ||
| self._populate_local_span_data(span) | ||
|
|
||
| if "rabbitmq" in self.data and self.data["rabbitmq"]["sort"] == "consume": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked span.py and it seems like the _populate_entry_span_data & _populate_exit_span_data for rabbitmq is the same. In this case, we can let it stay in the ENTRY_SPANS tuple and just change the method here to publish and set self.k = 2
| "mongo", "mysql", "postgres", "rabbitmq", "redis", "rpc-client", "sqlalchemy", | ||
| "soap", "tornado-client", "urllib3", "pymongo", "gcs", "gcps-producer") | ||
|
|
||
| ENTRY_SPANS = ("aiohttp-server", "aws.lambda.entry", "celery-worker", "django", "wsgi", "rabbitmq", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep this. See my other comment below
| python37: | ||
| docker: | ||
| - image: circleci/python:3.7.9 | ||
| - image: circleci/postgres:9.6.5-alpine-ram | ||
| - image: circleci/mariadb:10-ram | ||
| - image: circleci/redis:5.0.4 | ||
| - image: rabbitmq:3.5.4 | ||
| - image: circleci/mongo:4.2.3-ram | ||
| - image: singularities/pubsub-emulator | ||
| environment: | ||
| PUBSUB_PROJECT_ID: "project-test" | ||
| PUBSUB_LISTEN_ADDRESS: "0.0.0.0:8432" | ||
| working_directory: ~/repo | ||
| steps: | ||
| - checkout | ||
| - pip-install-deps | ||
| - run: | ||
| name: run tests | ||
| environment: | ||
| INSTANA_TEST: "true" | ||
| command: | | ||
| . venv/bin/activate | ||
| pytest -v | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if we can run asqynp on Python 3.8 - if not, then we can have this additional test job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like support only until Python 3.7: benjamin-hodgson/asynqp#104
We can keep it @pdimitra
manojpandey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Closes #301