Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Full mode for SQL Server #7186
Full mode for SQL Server #7186
Changes from all commits
7bff59f
27f8857
b2698c8
5540a39
33fde67
d1cb730
d283d0a
fce6234
bb29e39
e200b53
cf4e7b2
df4fc90
20fb17f
c4aa15e
1953540
98d4bfa
33dc8b0
b7b2ba7
55d621b
e040d58
c4ae679
81e0c1a
baa05a9
5d816f8
9a9ec00
32d383b
129af61
f459700
79551b3
20c90db
98d657c
88b7221
694fe60
b170a00
d9a1fed
6bdb1cf
fbe2b8a
40386f7
7c0e3d1
0d57bc1
10d5fd6
6b01b4d
69715da
0b19e31
c69b2a2
4a0c3b9
6706a0d
6bd51da
ed1b0d4
31ae1cf
933e813
ce8069a
bffed95
414f924
7b47f62
d580d01
c253b32
ba74ddf
70721bc
7c8a954
fcb763c
b5217d1
5de92f3
c3e9fa9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If you create a smaller try / finally just around instrumentationStatement, then you won't need the null handling.
But that's a bit of a matter of preference
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 believe we need then another try/catch/finally block, as the code before
instrumentationStatement = connection.prepareStatement(instrumentationSql);
also can throw exceptions we want to log. This would make the code more verbose. The current approach with the ugly(instrumentationStatement != null)
check seems less problematic. What do you prefer?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.
My preference is usually that there's a tight try / finally with the resource acquisition just before the try and the resource disposed of by itself in the finally. This code is mixing the handling of two different resources into the one try / finally structure which can be hard to follow.
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.
Ideally, finish needs to be in a finally block.
If close raises anything other than a SQLException, the span isn't being finished.
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.
Can I change
} catch (SQLException e) {
to} catch (Exception e) {
, and leaveinstrumentationSpan.finish();
where it is, instead? A (small) advantage would be to avoid nesting.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.
fixed as described above
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.
No, it has to be a finally block. catch ( Exception e ) doesn't catch everything, since there are other classes of Throwables.
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 opened a separate PR for this.