Added export of operation duration metric #3881
Conversation
| return self._exception | ||
|
|
||
| @dataclass | ||
| class AfterCommandExecutionEvent: |
There was a problem hiding this comment.
It would be easier to understand what this event handles if the name suggests it is related to metrics collection or if it might be used for other things as well - at least in the docstrings, we can add a mention of the metrics collection.
| return_value=collector | ||
| ): | ||
| # Create event dispatcher (real one, to test the full chain) | ||
| event_dispatcher = EventDispatcher() |
There was a problem hiding this comment.
It is not mandatory - in case it is not provided as input argument a default EventDispatcher instances will be created.
| with mock.patch.object( | ||
| recorder, '_get_or_create_collector', return_value=collector | ||
| ): | ||
| event_dispatcher = EventDispatcher() |
There was a problem hiding this comment.
I suggest to have some of the test (we can also look at them as examples how some features can be used or need to be configured) without pre-creating the event dispatcher and leave the init method to create the default instance - in this case the end result will be the same - instance of type EventDispatcher
| assert attrs['server.port'] == 6379 | ||
| assert attrs['db.namespace'] == '0' | ||
|
|
||
| def test_pipeline_transaction_emits_multi_command_name( |
There was a problem hiding this comment.
Isn't this covered in the test above? The difference between those two tests is that the first one sends several commands as transaction and this one sends just one command
| """ | ||
| Test that an empty pipeline (no commands) does not emit an event. | ||
| """ | ||
| from redis.client import Pipeline |
There was a problem hiding this comment.
I think all those imports can be removed - they are already added to the top level imports in the beginning of the file.
| ): | ||
| # Create a new event dispatcher and attach it to the cluster | ||
| event_dispatcher = EventDispatcher() | ||
| r._event_dispatcher = event_dispatcher |
There was a problem hiding this comment.
Is this needed? It should already be initialized. Or it needs to be initialized after OTelConfig?
Description of change
Please provide a description of the change here.
Pull Request check-list
Please make sure to review and check all of these items:
NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.