Skip to content
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 wrappers for redis.search methods create_index, search, aggregate #2635

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

mpozniak95
Copy link

@mpozniak95 mpozniak95 commented Jun 26, 2024

Description

Catch separately redisearch methods: redis.commands.search, create_index with attributes:

For create_index method: https://redis-py.readthedocs.io/en/stable/redismodules.html#redis.commands.search.commands.SearchCommands.create_index
Attributes:

  • fields - to show from which fields index was created

For search method: https://redis-py.readthedocs.io/en/stable/redismodules.html#redis.commands.search.commands.SearchCommands.search
Attributes:

  • query - shows query string
  • total - shows number of documents returned
  • xdoc{index} - shows returned documents

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

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

  • Added functional tests for these methods

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Copy link

linux-foundation-easycla bot commented Jun 26, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@mpozniak95 mpozniak95 changed the title [WIP] Add wrappers for redis.search methods create_index, search, aggregate Add wrappers for redis.search methods create_index, search, aggregate Aug 14, 2024
@mpozniak95 mpozniak95 marked this pull request as ready for review August 14, 2024 06:22
@mpozniak95 mpozniak95 requested a review from a team August 14, 2024 06:22
@lzchen
Copy link
Contributor

lzchen commented Aug 14, 2024

@mpozniak95

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?

@mpozniak95
Copy link
Author

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.

@lzchen
Copy link
Contributor

lzchen commented Aug 19, 2024

@mpozniak95

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).

Is the plan to add INTERNAL spans or should these calls be treated as DATABASE spans?

You are currently also creating INTERNAL spans for the current implementation. Is this correct or should we treating the spans as CLIENT with type database which follows database semantic conventions

@mpozniak95
Copy link
Author

mpozniak95 commented Aug 26, 2024

You are right the spans should be CLIENT type, I changed it. I also edited the description

@mpozniak95
Copy link
Author

I changed the implementation to catch those functions in _traced_execute_command and edit their span_name and attributes. Also for now I deleted the aggregate from the PR, I will add it in separate PR once we merge this.

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 _parse_response function in redis package) but execute_command returns unparsed response. I added some comments to the code (I hope they make any sense, maybe it will be easier to review)

_set_span_attribute(
span, "redis.search.total", number_of_returned_documents
)
if "NOCONTENT" in args:
Copy link
Member

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?

Copy link
Author

@mpozniak95 mpozniak95 Aug 30, 2024

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

Copy link
Contributor

@lzchen lzchen left a 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():
Copy link
Contributor

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"?

@lzchen lzchen requested a review from a team as a code owner September 18, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants