spring-cloud-sleuth-instrumentation sql issue for spans

[mschielmann]

Describe the bug
Version: 3.1.0
The TraceListenerStrategy in spring-cloud-sleuth-instrumentation throws a StringOutOfBoundsException when the SQL consists of only one word, like: BEGIN. Such word, afaik, constitutes a valid SQL statement, and as such should be handled without throwing an exception.

The exception is thrown here:
https://github.com/spring-cloud/spring-cloud-sleuth/blob/main/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/jdbc/TraceListenerStrategy.java#L432
(in spanName method), as indexOf(' ') is -1.

I'd suggest handling this with returning the whole SQL as span name in such case. Another option would be to ignore such SQL.

Sample
N/A

[marcingrzejszczak]

For sure we shouldn't throw an exception. Yeah returning the whole sql would be fine (I understand that this would happen only in case when sql would have only one word, right?). Wdyt @gavlyukovskiy ?

[mschielmann]

@marcingrzejszczak answering your question, yes, that would be the case. I don't think any validation is possible though as to if that word can create a valid SQL statement by itself (for example 'BEGINN').

If you agree that this can be fixed by using the whole word - then I could create a MR for that.

[marcingrzejszczak]

Be my guest!

[mschielmann]

@marcingrzejszczak, waiting for your review.
#2086

[gavlyukovskiy]

In my starter I didn't try to parse query to give span a name, so all of them were query, there are certainly a lot of edge cases when trying to parse user input, couple that comes to mind

• Not terminated by space: select*from .... Can be solved by query.split("[^a-zA-Z]", 2)[0]
• Has a comment in the beginning: /*some comment*/select ...
• Not an edge case, but in certain queries first (non-comment) word does not identify the query: with ... [select|insert|...]

Not sure what would be the best way to handle those, what do you think about checking that first word is select/insert/update/delete and using it as a span name, but in all other cases use something generic (like query) as a span name.

[mschielmann]

@gavlyukovskiy sounds reasonable to me, I'm happy to make that change to PR if you think that would be a best way to go.

[marcingrzejszczak]

I like @gavlyukovskiy comment. Let's do it like he suggests.

[gavlyukovskiy]

@marcingrzejszczak @mschielmann great, let's do that then.

[mschielmann]

@gavlyukovskiy @marcingrzejszczak I've applied changes to my PR. Please let me know if you think that any changes are required.
Thank you for your input.

[mschielmann]

@gavlyukovskiy @marcingrzejszczak,
my PR has been merged before I could apply changes based on your comments @gavlyukovskiy (busy time, holidays, sorry).

I've created another PR with some improvements 'based' on your comments. Please see if that works for you or let me know if you think something more could be added / changed.

#2091

Thanks,
MS

[marcingrzejszczak]

Oh sorry, my bad. Thanks for the pr!

[mschielmann]

Come to think about it, we're now changing the behaviour a bit - if someone depended on spanName like 'alter' or 'create' then we'd break things for them. So I'd either go back to returning the first word (whatever it may be, by removing the 'known span names') or by adding alter/create/drop and any other that may come to our minds.

[marcingrzejszczak]

Good catch! I'd prefer not to introduce any breaking changes. Maybe it makes more sense to leave the first word? WDYT @gavlyukovskiy ?