-
Notifications
You must be signed in to change notification settings - Fork 315
add a feature flag to disable extra calls to the DB in JDBC instrumentation #9774
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
Draft
vandonr
wants to merge
7
commits into
master
Choose a base branch
from
vandonr/jdbc-ff
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+277
−24
Draft
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e29146f
add config flags but in the wrong place
vandonr a685b20
move flags
vandonr 1b7f677
read config only once
vandonr cd6e512
remove spotless:off comment
vandonr ea2c982
add test
vandonr 4a1d0cb
add debug logs in error cases
vandonr 9ac990b
Merge remote-tracking branch 'origin/master' into vandonr/jdbc-ff
vandonr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
170 changes: 170 additions & 0 deletions
170
dd-java-agent/instrumentation/jdbc/src/test/groovy/JDBCDecoratorParseDBInfoTestBase.groovy
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| import static datadog.trace.api.config.TraceInstrumentationConfig.DB_CLIENT_INFO_FETCHING_ENABLED | ||
| import static datadog.trace.api.config.TraceInstrumentationConfig.DB_METADATA_FETCHING_ENABLED | ||
|
|
||
| import datadog.trace.agent.test.InstrumentationSpecification | ||
| import datadog.trace.bootstrap.instrumentation.jdbc.DBInfo | ||
| import datadog.trace.instrumentation.jdbc.JDBCDecorator | ||
| import java.sql.Connection | ||
| import test.TestDatabaseMetaData | ||
|
|
||
| /** | ||
| * Base test class for parseDBInfoFromConnection with different flag configurations | ||
| */ | ||
| abstract class JDBCDecoratorParseDBInfoTestBase extends InstrumentationSpecification { | ||
|
|
||
| def "test parseDBInfoFromConnection with null connection"() { | ||
| when: | ||
| def result = JDBCDecorator.parseDBInfoFromConnection(null) | ||
|
|
||
| then: | ||
| result == DBInfo.DEFAULT | ||
| } | ||
|
|
||
| def "test parseDBInfoFromConnection with null ClientInfo"() { | ||
| setup: | ||
| def metadata = new TestDatabaseMetaData() | ||
| metadata.setURL("jdbc:postgresql://testhost:5432/testdb") | ||
| def connection = Mock(Connection) { | ||
| getMetaData() >> metadata | ||
| getClientInfo() >> null | ||
| } | ||
|
|
||
| when: | ||
| def result = JDBCDecorator.parseDBInfoFromConnection(connection) | ||
|
|
||
| then: | ||
| if (shouldFetchMetadata()) { | ||
| result.type == "postgresql" | ||
| result.host == "testhost" | ||
| result.port == 5432 | ||
| result.db == "testdb" | ||
| } else { | ||
| result == DBInfo.DEFAULT | ||
| } | ||
| } | ||
|
|
||
| def "test parseDBInfoFromConnection regular case"() { | ||
| setup: | ||
| def metadata = new TestDatabaseMetaData() | ||
| metadata.setURL("jdbc:postgresql://testhost:5432/testdb") | ||
| def clientInfo = new Properties() | ||
| clientInfo.setProperty("warehouse", "my-test-warehouse") // we'll check that property to know if clientInfo were used | ||
| def connection = Mock(Connection) { | ||
| getMetaData() >> (shouldFetchMetadata() ? metadata : { assert false }) | ||
| getClientInfo() >> (shouldFetchClientInfo() ? clientInfo : { assert false }) | ||
| } | ||
|
|
||
| when: | ||
| def result = JDBCDecorator.parseDBInfoFromConnection(connection) | ||
|
|
||
| then: | ||
| if (shouldFetchMetadata()) { | ||
| result.type == "postgresql" | ||
| result.host == "testhost" | ||
| result.port == 5432 | ||
| result.db == "testdb" | ||
| if (shouldFetchClientInfo()) { // only if we _also_ fetch metadata | ||
| result.warehouse == "my-test-warehouse" | ||
| } | ||
| else { | ||
| result.warehouse == null | ||
| } | ||
| } else { | ||
| result == DBInfo.DEFAULT | ||
| } | ||
| } | ||
|
|
||
| abstract boolean shouldFetchMetadata() | ||
|
|
||
| abstract boolean shouldFetchClientInfo() | ||
| } | ||
|
|
||
| /** | ||
| * Test with both flags enabled (default behavior) | ||
| */ | ||
| class JDBCDecoratorParseDBInfoWithMetadataAndClientInfoForkedTest extends JDBCDecoratorParseDBInfoTestBase { | ||
|
|
||
| @Override | ||
| void configurePreAgent() { | ||
| super.configurePreAgent() | ||
| injectSysConfig(DB_METADATA_FETCHING_ENABLED, "true") | ||
| injectSysConfig(DB_CLIENT_INFO_FETCHING_ENABLED, "true") | ||
| } | ||
|
|
||
| @Override | ||
| boolean shouldFetchMetadata() { | ||
| return true | ||
| } | ||
|
|
||
| @Override | ||
| boolean shouldFetchClientInfo() { | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Test with both flags disabled | ||
| */ | ||
| class JDBCDecoratorParseDBInfoWithoutCallsForkedTest extends JDBCDecoratorParseDBInfoTestBase { | ||
|
|
||
| @Override | ||
| void configurePreAgent() { | ||
| super.configurePreAgent() | ||
| injectSysConfig(DB_METADATA_FETCHING_ENABLED, "false") | ||
| injectSysConfig(DB_CLIENT_INFO_FETCHING_ENABLED, "false") | ||
| } | ||
|
|
||
| @Override | ||
| boolean shouldFetchMetadata() { | ||
| return false | ||
| } | ||
|
|
||
| @Override | ||
| boolean shouldFetchClientInfo() { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Test with metadata enabled but client info disabled | ||
| */ | ||
| class JDBCDecoratorParseDBInfoWithMetadataOnlyForkedTest extends JDBCDecoratorParseDBInfoTestBase { | ||
|
|
||
| @Override | ||
| void configurePreAgent() { | ||
| super.configurePreAgent() | ||
| injectSysConfig(DB_METADATA_FETCHING_ENABLED, "true") | ||
| injectSysConfig(DB_CLIENT_INFO_FETCHING_ENABLED, "false") | ||
| } | ||
|
|
||
| @Override | ||
| boolean shouldFetchMetadata() { | ||
| return true | ||
| } | ||
|
|
||
| @Override | ||
| boolean shouldFetchClientInfo() { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| class JDBCDecoratorParseDBInfoWithClientInfoOnlyForkedTest extends JDBCDecoratorParseDBInfoTestBase { | ||
|
|
||
| @Override | ||
| void configurePreAgent() { | ||
| super.configurePreAgent() | ||
| injectSysConfig(DB_METADATA_FETCHING_ENABLED, "false") | ||
| injectSysConfig(DB_CLIENT_INFO_FETCHING_ENABLED, "true") | ||
| } | ||
|
|
||
| @Override | ||
| boolean shouldFetchMetadata() { | ||
| return false | ||
| } | ||
|
|
||
| @Override | ||
| boolean shouldFetchClientInfo() { | ||
| return true | ||
| } | ||
| } | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
Maybe make sense to put some debug/trace log? Just thinking out loud.
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.
you mean for investigation in the current case, or in general ?
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 think it could help for investigation in the current case! I have no scale for how often this method is called though -- would adding debug statements here be "okay" in a production environment where we don't want to spam logs?
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.
added a log, we'll need to make sure this is not merged to main if we want to keep the feature flags for further investigations
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.
Cool thanks! I'll add a
do not mergetag for now to remind us