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

Introducing the Observer pattern to SQL execution #2939

Open
wants to merge 2 commits into
base: 4.9.x
Choose a base branch
from

Conversation

lcavadas
Copy link
Contributor

Externalising the QUERY_LOG as an observer so the same mechanism can be used for adding the query information to Spans (tracing)

@graemerocher
Copy link
Contributor

Seems like a good idea but needs tests

@lcavadas
Copy link
Contributor Author

@graemerocher Added tests for the usual suspects (CRUD) and also added a test for @Query annotated methods.

@graemerocher
Copy link
Contributor

I wonder if it makes sense to instrument the DataSource rather than the repository implementation //cc @dstepanov @radovanradic

@dstepanov
Copy link
Contributor

That would require introspecting statements. We have some other issue requesting combining parameters binding logging with the query, so maybe this can be combined.

@graemerocher
Copy link
Contributor

good point, but we should probably at least enhance JdbcOperations to log no?

@dstepanov
Copy link
Contributor

What do you mean by enhance?

@lcavadas
Copy link
Contributor Author

@dstepanov I took a peek at that other issue you mentioned and while this mechanism would definitely work for that we would need to add some sort of trigger/done function to the observers in order to perform the actual logging. As I am not that familiar with this codebase I am unsure of where that trigger should actually happen. That said I'm happy to add it if you point me in the general direction of where you think it would be best to do it.

@CLAassistant
Copy link

CLAassistant commented Sep 3, 2024

CLA assistant check
All committers have signed the CLA.

@lcavadas lcavadas changed the base branch from 4.8.x to 4.9.x September 3, 2024 11:57
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