Follow-up on the apparently abbandonned https://github.com/open-telemetry/opentelemetry-python-contrib/pull/2114.
The asyncpg instrumentation attempts to fall back on using the database
name as the span name in case the first argument to the instrumented
method is falsey.
This has probably never worked since asyncpg defines the `_params`
attribute as an instance of `ConnectionParams`
(https://github.com/MagicStack/asyncpg/blob/master/asyncpg/connection.py#L62)
which is a NamedTuple instance and thus don't define `get`. The proper
way of safely accessing properties on a NamedTuple is using `getattr`.
The only case that I've actually found which triggers this branch is if
the supplied query is an empty string. This is something that causes an
`AttributeError` for `Connection.execute` but is fine for `fetch()`,
`fetchval()`, `fetchrow()` and `executemany()`.
The tests have been expanded to check these cases. Also, more status
code validation has been added where it was missing.
Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
* Unwrap `ExceptionInfo` and `ExceptionWithTraceback`
Instead of reporting the `ExceptionInfo` and `ExceptionWithTraceback`
wrappers raised by Celery, report the exceptions that they wrap.
This ensures that the exception in the OpenTelemetry span has a type
and traceback that are meaningful and relevant to the developer.
* Fix typo
The exception is expected, not excepted. Well, I guess it is also
excepted, because it's an exception, but you get what I mean.
* Reformat file with `black`
Reformat the `__init__.py` file in the Celery instrumentation using
`black`, fixing a CI linter error.
* Address review feedback
Use the VERSION attribute exposed by Billiard to decide whether to
import ExceptionWithTraceback.
Add a test for a failing task and check that the exceptions' type
and message are preserved.
* Amend ExceptionWithTraceback import
* Add Redis instrumentation query sanitization
Add a query sanitizer to the Redis instrumentation. This can be disabled
with the `sanitize_query = False` config option.
Given the query `SET key value`, the sanitized query becomes `SET ? ?`.
Both the keys and values are sanitized, as both can contain PII data.
The Redis queries are sanitized by default. This changes the default
behavior of this instrumentation. Previously it reported unsanitized
Redis queries.
This was previously discussed in the previous implementation of this PR
in PR #1571Closes#1548
* Update Redis sanitize_query option documentation
Changes suggested in
https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1572#discussion_r1066069584
* Remove uninstrument & instrument from test setup
The Redis test that performs the tests with the default options, doesn't
need to uninstrument and then instrument the instrumentor. This commit
removes the unnecessary setup code. The setup code is already present at
the top of the file.
* Fix code style formatting
* Update Redis functional tests
- Update the sanitizer to also account for a max `db.statement`
attribute value length. No longer than 1000 characters.
- Update the functional tests to assume the queries are sanitized by
default.
- Add new tests that test the behavior with sanitization turned off.
Only for the tests in the first test class. I don't think it's needed
to duplicate this test for the clustered and async setup combinations.
* Test Redis unsanitized queries by default
Change the Redis functional tests so that they test the unsanitized
query by default, and test the sanitized query results in the separate
test functions.
This is a partial revert of the previous commit
8d56c2f72e12c7d7dc4ef25a9fe6a69ea685a6d8
* Fix formatting issue in Redis utils
* Disable Redis query sanitization by default
Update the Redis instrumentation library to not change the default
behavior for the Redis instrumentation. This can be enabled at a later
time when the spec discussion about this topic has concluded.
https://github.com/open-telemetry/opentelemetry-specification/issues/3104
* Fix pylint issue
Remove else statement.
* Update changelog about Redis query sanitization default
[ci skip]
Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
* Fix potential error on Redis args being 0
Check the length of the args array and return an empty string if there
are no args.
That way it won't cause an IndexError if the args array is empty and it
tries to fetch the first element, which should be the Redis command.
---------
Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
* Add tests for errors in Celery tasks
I noticed there were no tests for the error scenario in the Celery
package. This commit adds a basic test, based on the previous test and
how I see other packages test the error status on the span.
Part of #987
* Record exception in Celery instrumentation
In addition to setting the status on the span, also record the exception
on the span. This adds an event to the span with more details about the
error, following the format other instrumentations also use.
* Update CHANGELOG with Celery record exception
* Fix lint code formatting issues
* Move Celery error tests to the functional tests
The celery tests failed on Python 3.11. This is most likely due to this
issue in billiard, a celery dependency, about it not working on Python
3.11 because of the error reported in the CI:
https://github.com/celery/billiard/issues/377
It's been fixed in billiard 4.1.0, but celery is locked on billiard
version lower than 4, so it cannot use this version with the fix.
This issue does not arise on the Docker tests, because they use Python
3.9.16.
I've moved the error test span event assertions to the error test that
is available in the functional tests, and removed the unit test. That
way, the build will run successfully.
* Remove duplicate entry in changelog
This was added in a recent merge commit on this PR branch.
* Remove unused test code
With the move of the tests for tasks with errors to the functional
tests, remove the unit test's error task and unused imports.
---------
Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
* Fix install of Python 3.10 on GitHub Actions
In PR #1604 the Python version was upgraded to Python 3.10 to fix a
local issue on M1 MacBooks.
The GitHub Action workflows now exit with the following message for the
docker-tests, spellcheck and lint checks, skipping these checks.
```
lint create: /home/runner/work/opentelemetry-python-contrib/opentelemetry-python-contrib/.tox/lint
SKIPPED: InterpreterNotFound: python3.10
___________________________________ summary ____________________________________
SKIPPED: lint: InterpreterNotFound: python3.10
congratulations :)
```
Upgrade the Python version in the GitHub Actions workflow to fix this.
* Fix YAML interpretation of Python 3.10
* Upgrade Docker tests dependencies
Upgrade the asyncpg and psycopg2 packages, they don't work on Python
3.10.
This also fixes running these tests no M1 MacBooks.
* Fix linter issues merged into main
They went unnoticed while the CI didn't fail on the lint task not
working.
---------
Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
Fixes https://github.com/open-telemetry/opentelemetry-python-contrib/issues/1353
Also:
Fix the check for the connection already being instrumented in instrument_connection()
Add tests for commit() and rollback()
Add a couple missing docstring items.
Add basepython to docker-tests to fix running the tests on macOS.
* Refactor code using pyupgrade for Python 3.6
This diff is the result of applying the following command to the
project:
```shell
find . -type f -name "*.py" -exec pyupgrade --py36-plus '{}' +
```
* Simplify yield from
* Remove unneded CI steps
These were not really needed and got in by mistake.
* Fix broken mssql integration tests
We were giving mssql server 10 seconds to start before creating the test
database. It now takes Github CI more than 10 seconds to start the mssql
server. Instead of increasing the leeway with guesses, this commit moves
the creation of test database from docker compose to the python test suite.
This allows the docker image to come up first and then create the DB
inside the test suite with proper retry mechanism that is already in
place.
The test was actually generating 2 spans but on very fast systems we
only saw one as the test ran faster than the message would travel
through the broker, trigger a task and generate 2nd span.
Switched from .delay() to .apply() so only one span is generated which
is enough for the test in question.
It seems the same in memory span exporter was being reused by multiple
tests. This change _should_ create a new instance of memeory exporter
per test/function.
Fixes#2067
* redis: fix default port KeyError, wrong attr name
* fix docker tests and another ip/port issue in asyncpg
Co-authored-by: Alex Boten <aboten@lightstep.com>
