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

Hibernate query span naming #3106

Merged
merged 5 commits into from
May 28, 2021
Merged

Conversation

laurit
Copy link
Contributor

@laurit laurit commented May 27, 2021

Currently the span name for hibernate queries created with createQuery and createSQLQuery is the query itself. This query can contain high cardinality components and unsanitized info. This pr changes span name for hibernate queries to follow the same pattern as jdbc queries operation + entity/table name.
Additionally tests that were not present for hibernate 4.3 were copied from 4.0. Hibernate 6 tests were moved to a separate directory as there are some api changes due to which most of the tests won't work on hibernate 6.

Comment on lines 63 to 67
// query might be hql or jpql query where select is optional, prepend select so we can use
// sql sanitizer to extract entity/table name
if (queryString.trim().toUpperCase(Locale.ROOT).startsWith("FROM ")) {
queryString = "SELECT * " + query;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea: we could change the AutoSqlSanitizer so that it uses Select operation if we encountered FROM and there's no operation set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Can you add some tests to SqlStatementSanitizerTest?

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally tests that were not present for hibernate 4.3 were copied from 4.0.

hibernate-4.0 instrumentation applies to 4.0+ (no upper bound), what do you think about moving the hibernate 6 tests to the hibernate-4.0 module, and rename the hibernate-4.3 module to hibernate-procedure-call-4.3 (and only have tests for hibernate ProcedureCall in that module)?

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@trask trask merged commit 35d6bdb into open-telemetry:main May 28, 2021
@laurit laurit deleted the hibernate-query-span branch June 7, 2021 11:50
robododge pushed a commit to robododge/opentelemetry-java-instrumentation that referenced this pull request Jun 17, 2021
* Hibernate query span naming

* remove commented out code

* modify query sanitizer to accept queries that start with from clause

* add sql sanitizer test for queries starting with from

* rename hibernate-4.3 to hibernate-procedure-call-4.3
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.

3 participants