Refactored createSqlSubsegment into a public util file #186
Conversation
aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/SqlUtils.java
Outdated
Show resolved
Hide resolved
aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/SqlUtils.java
Outdated
Show resolved
Hide resolved
aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/SqlUtils.java
Outdated
Show resolved
Hide resolved
aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/SqlUtils.java
Outdated
Show resolved
Hide resolved
| DatabaseMetaData metadata = connection.getMetaData(); | ||
| String subsegmentName = DEFAULT_DATABASE_NAME; | ||
| try { | ||
| URI normalizedUri = new URI(new URI(metadata.getURL()).getSchemeSpecificPart()); |
There was a problem hiding this comment.
Optional because just copy paste, but I can't find a difference between this double-parse and just metadata.getHost()
There was a problem hiding this comment.
The JDBC DatabaseMetaData interface doesn't have a getHost() method, but I'm not sure why the getSchemeSpecificPart is necessary. I'll leave it as is to be safe, maybe @shengxil can shed some light.
| URI normalizedUri = new URI(new URI(metadata.getURL()).getSchemeSpecificPart()); | ||
| subsegmentName = connection.getCatalog() + "@" + normalizedUri.getHost(); | ||
| } catch (URISyntaxException e) { | ||
| logger.warn("Unable to parse database URI. Falling back to default '" + DEFAULT_DATABASE_NAME |
There was a problem hiding this comment.
If we could simplify the above this warning could go away, I think it's bad to spam this for every query.
There was a problem hiding this comment.
Agree we shouldn't be spamming this, but seeing as we're keeping the parsing for now I'll change it to debug
aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/SqlUtils.java
Outdated
Show resolved
Hide resolved
| import org.mockito.Mock; | ||
| import org.mockito.runners.MockitoJUnitRunner; | ||
|
|
||
| @RunWith(MockitoJUnitRunner.class) |
There was a problem hiding this comment.
Prefer rule, @Rule public MockitoRule = MockitoJunit.rue()
There was a problem hiding this comment.
I tried to do this but kept getting NPEs where I was stubbing methods because the mocks weren't initialized. I tried to also use ExtendWith like you did in some other classes, but that caused a weird NoSuchMethodError. I ended up settling on the static initMocks method in the BeforeEach block.
aws-xray-recorder-sdk-sql/src/test/java/com/amazonaws/xray/sql/SqlUtilsTest.java
Outdated
Show resolved
Hide resolved
aws-xray-recorder-sdk-sql/src/test/java/com/amazonaws/xray/sql/SqlUtilsTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Addressed the comments, and upgraded the tests to JUnit 5. This caused some weird errors that prevented me from adopting @rule annotation, let me know if there's a better fix than what I did.
aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/SqlUtils.java
Outdated
Show resolved
Hide resolved
| DatabaseMetaData metadata = connection.getMetaData(); | ||
| String subsegmentName = DEFAULT_DATABASE_NAME; | ||
| try { | ||
| URI normalizedUri = new URI(new URI(metadata.getURL()).getSchemeSpecificPart()); |
There was a problem hiding this comment.
The JDBC DatabaseMetaData interface doesn't have a getHost() method, but I'm not sure why the getSchemeSpecificPart is necessary. I'll leave it as is to be safe, maybe @shengxil can shed some light.
| URI normalizedUri = new URI(new URI(metadata.getURL()).getSchemeSpecificPart()); | ||
| subsegmentName = connection.getCatalog() + "@" + normalizedUri.getHost(); | ||
| } catch (URISyntaxException e) { | ||
| logger.warn("Unable to parse database URI. Falling back to default '" + DEFAULT_DATABASE_NAME |
There was a problem hiding this comment.
Agree we shouldn't be spamming this, but seeing as we're keeping the parsing for now I'll change it to debug
aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/SqlUtils.java
Outdated
Show resolved
Hide resolved
aws-xray-recorder-sdk-sql/src/main/java/com/amazonaws/xray/sql/SqlUtils.java
Outdated
Show resolved
Hide resolved
aws-xray-recorder-sdk-sql/src/test/java/com/amazonaws/xray/sql/SqlUtilsTest.java
Outdated
Show resolved
Hide resolved
aws-xray-recorder-sdk-sql/src/test/java/com/amazonaws/xray/sql/SqlUtilsTest.java
Outdated
Show resolved
Hide resolved
| import org.mockito.Mock; | ||
| import org.mockito.runners.MockitoJUnitRunner; | ||
|
|
||
| @RunWith(MockitoJUnitRunner.class) |
There was a problem hiding this comment.
I tried to do this but kept getting NPEs where I was stubbing methods because the mocks weren't initialized. I tried to also use ExtendWith like you did in some other classes, but that caused a weird NoSuchMethodError. I ended up settling on the static initMocks method in the BeforeEach block.
|
|
||
| @BeforeEach | ||
| void setup() throws SQLException { | ||
| MockitoAnnotations.initMocks(this); |
There was a problem hiding this comment.
Hmm surprised extends with didn't work will take a look after the PR
Description of changes:
In order to reuse the subsegment creation logic in the Agent's SQL auto-instrumentation, I moved the private
createSubsegmentmethod into a new public util class. I made the new Util class rather than just making the method public inTracingStatementsince we've deprecated TracingStatement, so I thought this public method should be separate.I also added a capability to record SQL queries, relating to #28. Including them in subsegments will be configurable for the agent, but no such config exists in the SDK yet.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.