-
Notifications
You must be signed in to change notification settings - Fork 584
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
Add additional attributes for redis.search methods create_index, search #2635
base: main
Are you sure you want to change the base?
Add additional attributes for redis.search methods create_index, search #2635
Conversation
...entation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py
Outdated
Show resolved
Hide resolved
I am not too familiar with the inner workings of Redis. Could you update the description of the PR to be more descriptive of why are we making these changes? Is the plan to add INTERNAL spans or should these calls be treated as DATABASE spans? |
Idea is to integrate it with: https://github.com/traceloop/openllmetry. Since those three methods are probably the most used Redis methods in LLM applications we would like to have additional attributes in spans of that methods. |
...entation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py
Outdated
Show resolved
Hide resolved
Please update the description of the pr to reflect what you are trying to do. I am not familiar with the inner workings of https://github.com/traceloop/openllmetry so please summarize exactly what you are trying to do and what new methods you are trying to trace (linking to redis docs would be most helpful as well).
You are currently also creating |
You are right the spans should be |
I changed the implementation to catch those functions in I feel that it got a little complicated right now, it was easy to extract the response from redis.commands.search function directly (because it was already parsed by |
...tion/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py
Outdated
Show resolved
Hide resolved
_set_span_attribute( | ||
span, "redis.search.total", number_of_returned_documents | ||
) | ||
if "NOCONTENT" in args: |
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.
do we also need to early return if not number_of_returned_documents?
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, then we don't need another if in line 285. I changed it in last commit
…TYPES, return when zero returned docs
...entation/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/util.py
Outdated
Show resolved
Hide resolved
...tion/opentelemetry-instrumentation-redis/src/opentelemetry/instrumentation/redis/__init__.py
Outdated
Show resolved
Hide resolved
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. @mpozniak95 Please add a CHANGELOG entry and we can get this merged!
if callable(request_hook): | ||
request_hook(span, instance, args, kwargs) | ||
response = func(*args, **kwargs) | ||
if span.is_recording(): |
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.
Nit: Can't we include this as part of the above "if"?
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 think no, because one of the additional attributes is based on the response of search method so we need this to be placed after:
response = func(*args, **kwargs)
which as I understand should happen even if span is not recording
So, can we merge it :)? |
Description
Add additional attributes for redisearch methods: redis.commands.search, create_index:
For create_index method: https://redis-py.readthedocs.io/en/stable/redismodules.html#redis.commands.search.commands.SearchCommands.create_index
Attributes:
For search method: https://redis-py.readthedocs.io/en/stable/redismodules.html#redis.commands.search.commands.SearchCommands.search
Attributes:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.