feat: Add OpenTelemetry instrumentation #525
Conversation
…hecks from pre-commit, fix taskiq-python#417
| content: "97DC185FE0A2F5B123861F0790FDFB26" # pragma: allowlist secret | ||
| - - meta | ||
| - name: "yandex-verification" | ||
| content: "9b105f7c58cbc920" |
There was a problem hiding this comment.
Why pragma: allowlist secret is here?
As I see, we already exclude this file from detect-secrets pre-commit hook check. Is some other linter checks fails on this lines?
There was a problem hiding this comment.
even with exclusion in .pre-commit-config.yaml, still seeing this without pragmas
Detect secrets...........................................................Failed
- hook id: detect-secrets
- exit code: 1
ERROR: Potential secrets about to be committed to git repo!
Secret Type: Base64 High Entropy String
Location: docs\README.md:17
Secret Type: Hex High Entropy String
Location: docs\README.md:20
Secret Type: Hex High Entropy String
Location: docs\README.md:23
Possible mitigations:
- For information about putting your secrets in a safer place, please ask in
#security
- Mark false positives with an inline `pragma: allowlist secret` comment
If a secret has already been committed, visit
https://help.github.com/articles/removing-sensitive-data-from-a-repositoryThere was a problem hiding this comment.
Thats strange...
I checked on my fork and anything seems to be fine:
(taskiq) git pull
Already up to date.
(taskiq) git st
On branch master
Your branch is up to date with 'origin/master'.
nothing to commit, working tree clean
(taskiq) pre-commit install
pre-commit installed at .git/hooks/pre-commit
(taskiq) pre-commit run detect-secrets --all-files
Detect secrets...........................................................Passed
Are you sure that you run poetry install --all-extras && pre-commit install after rebase? I don't really know that else can be a problem
There was a problem hiding this comment.
In any case - it's not a blocker to merge this MR. I will deal with it later if we don't find the root cause of this strange pre-commit hook behaviour.
There was a problem hiding this comment.
I guess the root cause is me commiting from windows :)
On mac the exclusions are working as expected
taskiq/package.py
Outdated
| @@ -0,0 +1,2 @@ | |||
| # for compatibility with opentelemetry-instrumentation | |||
| _instruments = ("taskiq >= 0.11.19",) | |||
There was a problem hiding this comment.
Just wondering: do we need to update version in this file on every release?
tests/opentelemetry/test_tasks.py
Outdated
|
|
||
| self.assertEqual(result.return_value, {"key": "value"}) | ||
|
|
||
| @pytest.mark.anyio |
There was a problem hiding this comment.
I enabled anyio_mode = "auto" flag for pytest. So tests doesn't need to explicitly say that they are async with pytest.mark.anyio marker.
You can just remove it and tests should work as they were)
taskiq/instrumentation.py
Outdated
| --- | ||
| """ | ||
|
|
||
| from __future__ import annotations |
There was a problem hiding this comment.
Seems like everything works fine without this import. Maybe we can remove it?
As far as I can see, there are no other places in the taskiq repo where __future__ is used.
taskiq/instrumentation.py
Outdated
| broker = InMemoryBroker() | ||
| @broker.on_event(TaskiqEvents.WORKER_STARTUP) | ||
| async def startup(state: TaskiqState) -> None: |
There was a problem hiding this comment.
Can you do it in main? So you won't need to create to on_event.
| if not span.is_recording(): | ||
| return | ||
|
|
||
| for key in TASKIQ_CONTEXT_ATTRIBUTES: |
There was a problem hiding this comment.
Why do we track only particular labels? Let's just iterate over all available labels.
Also, I think that we also want to extract args and kwargs to set it on the span. For better tracing. Instead of passing labels, you can pass TaskiqMessage itself and get everthing from it.
There was a problem hiding this comment.
AttributeValue is very simple type.
So if we want to include args and kwargs in traces, we need to serialize them.
Also there is schedule label which is a list[dict[...]]
Maybe just add hook parameters to the middleware, so user can handle this himself?
There was a problem hiding this comment.
🤔 🤔 🤔
Let's leave it as it is right now. We might reconsider it later.
| ctx = retrieve_context(message, is_publish=True) | ||
|
|
||
| if ctx is None: | ||
| logger.warning("no existing span found for task_id=%s", message.task_id) |
There was a problem hiding this comment.
| logger.warning("no existing span found for task_id=%s", message.task_id) | |
| logger.debug("no existing span found for task_id=%s", message.task_id) |
middleware post_send, post_execute, post_save, on_error are now called in reverse order
taskiq/instrumentation.py
Outdated
| self.instrument_broker(broker) | ||
| return result | ||
|
|
||
| wrap_function_wrapper("taskiq", "AsyncBroker.__init__", broker_init) |
There was a problem hiding this comment.
It doesn't work with autoinstrumentation, because broker is not always from taskiq module.
To make it work, please update a path to the module.
| wrap_function_wrapper("taskiq", "AsyncBroker.__init__", broker_init) | |
| wrap_function_wrapper("taskiq.abc.broker", "AsyncBroker.__init__", broker_init) |
| if not span.is_recording(): | ||
| return | ||
|
|
||
| for key in TASKIQ_CONTEXT_ATTRIBUTES: |
There was a problem hiding this comment.
🤔 🤔 🤔
Let's leave it as it is right now. We might reconsider it later.
taskiq/instrumentation.py
Outdated
| *args: Any, | ||
| **kwargs: Any, | ||
| ) -> None: | ||
| import opentelemetry.instrumentation.auto_instrumentation.sitecustomize # noqa |
There was a problem hiding this comment.
| import opentelemetry.instrumentation.auto_instrumentation.sitecustomize # noqa | |
| from opentelemetry.instrumentation.auto_instrumentation import initialize | |
| initialize() |
taskiq/instrumentation.py
Outdated
|
|
||
| wrap_function_wrapper("taskiq", "AsyncBroker.__init__", broker_init) | ||
| wrap_function_wrapper("taskiq.abc.broker", "AsyncBroker.__init__", broker_init) | ||
| wrap_object_attribute( |
There was a problem hiding this comment.
Actually, I guess it might be simpler to wrap taskiq.cli.worker.run:start_listen. At least it won't require any partial.
3 participants
No description provided.