-
Notifications
You must be signed in to change notification settings - Fork 355
Decouple Kafka DSM instrumentation from APM tracing enablement #12350
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
ericfirth
wants to merge
24
commits into
master
Choose a base branch
from
worktree-dsm-kafka-decoupling
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.
Draft
Changes from all commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
14f3422
Add InstrumenterConfig.isDataStreamsEnabled() accessor
ericfirth bc5b126
Add TargetSystem.DATA_STREAMS and InstrumenterModule.DataStreams base…
ericfirth ab45b03
Add isEnabled() test coverage for DataStreams
ericfirth d93cdd3
Fix test isolation bug in testDataStreamsIsEnabledWhenDataStreamsEnab…
ericfirth 4b971cf
Remove unused AfterEach import from InstrumenterModuleTest
ericfirth e06d87a
Wire AgentInstaller.getEnabledSystems() to DATA_STREAMS target system
ericfirth 88d3ced
Add SamplingMechanism.DATA_STREAMS constant
ericfirth d415510
Fix DATA_STREAMS case formatting to match DATA_JOBS style
ericfirth 1f63ee4
Swap kafka-clients-0.11 instrumentations to InstrumenterModule.DataSt…
ericfirth 9d6e647
Suppress APM billing for DSM-only Kafka produce spans
ericfirth 0d7f5f8
Swap kafka-clients-3.8 instrumentations to InstrumenterModule.DataStr…
ericfirth 9c85a52
Decouple DSM from APM tracing in kafka-clients-3.8 (produce and consu…
ericfirth d4c4904
chore: kafka-streams-0.11 DSM decoupling (base-class swap + suppression)
ericfirth 4c77872
refactor: change kafka-streams-1.0 InternalTopologyBuilderInstrumenta…
ericfirth 4a3bcfb
refactor: decouple kafka-connect-0.11 instrumentation from APM tracing
ericfirth 8b7aebf
Migrate InstrumenterConfigTest to JUnit 5 and add isDataStreamsEnable…
ericfirth 25935aa
Migrate SamplingMechanismTest to JUnit 5 and add DATA_STREAMS cases
ericfirth f755f81
test: add JUnit 5 tests for AgentInstaller.getEnabledSystems() DATA_S…
ericfirth 3e47f9e
fix: suppress APM billing for DSM-only Kafka consume spans in kafka-c…
ericfirth 86764eb
Add tests for DSM billing suppression on kafka produce/consume spans
ericfirth dcce923
fix: only force DSM-only USER_DROP when the Kafka span owns the local…
ericfirth 3bad75e
fix: keep Kafka Code Origin instrumentation on the Tracing base class
ericfirth dd07f44
test: cover DSM-only sampling suppression for Kafka produce, consume …
ericfirth 8f5f263
fix: suppress DSM-only Kafka poll-span billing gap and address code r…
ericfirth 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
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
75 changes: 75 additions & 0 deletions
75
...taller/src/test/java/datadog/trace/agent/tooling/AgentInstallerGetEnabledSystemsTest.java
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,75 @@ | ||
| package datadog.trace.agent.tooling; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import datadog.trace.agent.tooling.InstrumenterModule.TargetSystem; | ||
| import datadog.trace.test.junit.utils.config.WithConfig; | ||
| import datadog.trace.test.junit.utils.config.WithConfigExtension; | ||
| import java.util.Set; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
|
|
||
| /** | ||
| * Tests for {@link AgentInstaller#getEnabledSystems()} to verify that it correctly includes target | ||
| * systems based on their corresponding configuration flags. | ||
| */ | ||
| @ExtendWith(WithConfigExtension.class) | ||
| class AgentInstallerGetEnabledSystemsTest { | ||
|
|
||
| /** | ||
| * Verifies that DATA_STREAMS target system is not included when data.streams.enabled is false | ||
| * (default). | ||
| */ | ||
| @Test | ||
| void dataStreamsNotIncludedWhenDisabled() { | ||
| Set<TargetSystem> enabledSystems = AgentInstaller.getEnabledSystems(); | ||
| assertFalse( | ||
| enabledSystems.contains(TargetSystem.DATA_STREAMS), | ||
| "DATA_STREAMS should not be included when disabled"); | ||
| } | ||
|
|
||
| /** Verifies that DATA_STREAMS target system is included when data.streams.enabled is true. */ | ||
| @Test | ||
| @WithConfig(key = "data.streams.enabled", value = "true") | ||
| void dataStreamsIncludedWhenEnabled() { | ||
| Set<TargetSystem> enabledSystems = AgentInstaller.getEnabledSystems(); | ||
| assertTrue( | ||
| enabledSystems.contains(TargetSystem.DATA_STREAMS), | ||
| "DATA_STREAMS should be included when enabled"); | ||
| } | ||
|
|
||
| /** Verifies that USM target system is not included when usm.enabled is false (default). */ | ||
| @Test | ||
| void usmNotIncludedWhenDisabled() { | ||
| Set<TargetSystem> enabledSystems = AgentInstaller.getEnabledSystems(); | ||
| assertFalse( | ||
| enabledSystems.contains(TargetSystem.USM), "USM should not be included when disabled"); | ||
| } | ||
|
|
||
| /** Verifies that USM target system is included when usm.enabled is true. */ | ||
| @Test | ||
| @WithConfig(key = "usm.enabled", value = "true") | ||
| void usmIncludedWhenEnabled() { | ||
| Set<TargetSystem> enabledSystems = AgentInstaller.getEnabledSystems(); | ||
| assertTrue(enabledSystems.contains(TargetSystem.USM), "USM should be included when enabled"); | ||
| } | ||
|
|
||
| /** Verifies that LLMOBS target system is not included when llmobs.enabled is false (default). */ | ||
| @Test | ||
| void llmobsNotIncludedWhenDisabled() { | ||
| Set<TargetSystem> enabledSystems = AgentInstaller.getEnabledSystems(); | ||
| assertFalse( | ||
| enabledSystems.contains(TargetSystem.LLMOBS), | ||
| "LLMOBS should not be included when disabled"); | ||
| } | ||
|
|
||
| /** Verifies that LLMOBS target system is included when llmobs.enabled is true. */ | ||
| @Test | ||
| @WithConfig(key = "llmobs.enabled", value = "true") | ||
| void llmobsIncludedWhenEnabled() { | ||
| Set<TargetSystem> enabledSystems = AgentInstaller.getEnabledSystems(); | ||
| assertTrue( | ||
| enabledSystems.contains(TargetSystem.LLMOBS), "LLMOBS should be included when enabled"); | ||
| } | ||
| } |
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
102 changes: 102 additions & 0 deletions
102
...agent/agent-tooling/src/test/java/datadog/trace/agent/tooling/InstrumenterModuleTest.java
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,102 @@ | ||
| package datadog.trace.agent.tooling; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import datadog.trace.agent.tooling.InstrumenterModule.TargetSystem; | ||
| import datadog.trace.test.junit.utils.config.WithConfig; | ||
| import datadog.trace.test.junit.utils.config.WithConfigExtension; | ||
| import java.util.HashSet; | ||
| import java.util.Set; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
|
|
||
| @ExtendWith(WithConfigExtension.class) | ||
| class InstrumenterModuleTest { | ||
|
|
||
| @Test | ||
| void testDataStreamsIsApplicableWithTracing() { | ||
| Set<TargetSystem> enabledSystems = new HashSet<>(); | ||
| enabledSystems.add(TargetSystem.TRACING); | ||
|
|
||
| InstrumenterModule.DataStreams module = new InstrumenterModule.DataStreams("test-module") {}; | ||
|
|
||
| assertTrue(module.isApplicable(enabledSystems)); | ||
| } | ||
|
|
||
| @Test | ||
| void testDataStreamsIsApplicableWithDataStreams() { | ||
| Set<TargetSystem> enabledSystems = new HashSet<>(); | ||
| enabledSystems.add(TargetSystem.DATA_STREAMS); | ||
|
|
||
| InstrumenterModule.DataStreams module = new InstrumenterModule.DataStreams("test-module") {}; | ||
|
|
||
| assertTrue(module.isApplicable(enabledSystems)); | ||
| } | ||
|
|
||
| @Test | ||
| void testDataStreamsIsApplicableWithBoth() { | ||
| Set<TargetSystem> enabledSystems = new HashSet<>(); | ||
| enabledSystems.add(TargetSystem.TRACING); | ||
| enabledSystems.add(TargetSystem.DATA_STREAMS); | ||
|
|
||
| InstrumenterModule.DataStreams module = new InstrumenterModule.DataStreams("test-module") {}; | ||
|
|
||
| assertTrue(module.isApplicable(enabledSystems)); | ||
| } | ||
|
|
||
| @Test | ||
| void testDataStreamsIsApplicableWithNeither() { | ||
| Set<TargetSystem> enabledSystems = new HashSet<>(); | ||
| enabledSystems.add(TargetSystem.APPSEC); | ||
|
|
||
| InstrumenterModule.DataStreams module = new InstrumenterModule.DataStreams("test-module") {}; | ||
|
|
||
| assertFalse(module.isApplicable(enabledSystems)); | ||
| } | ||
|
|
||
| @Test | ||
| @WithConfig(key = "trace.test-kafka-module.enabled", value = "false") | ||
| @WithConfig(key = "data.streams.enabled", value = "true") | ||
| void testDataStreamsIsEnabledWhenDataStreamsEnabledOverridesFalse() { | ||
| // When tracing for this integration is disabled but DSM is explicitly enabled, | ||
| // isEnabled() should still return true. | ||
| InstrumenterModule.DataStreams module = | ||
| new InstrumenterModule.DataStreams("test-kafka-module") {}; | ||
|
|
||
| assertTrue(module.isEnabled()); | ||
| } | ||
|
|
||
| @Test | ||
| @WithConfig(key = "trace.test-kafka-module.enabled", value = "true") | ||
| @WithConfig(key = "data.streams.enabled", value = "false") | ||
| void testDataStreamsIsEnabledWhenSuperEnabledIsTrue() { | ||
| // When super.isEnabled() is true, isEnabled() should return true regardless of DSM state. | ||
| InstrumenterModule.DataStreams module = | ||
| new InstrumenterModule.DataStreams("test-kafka-module") {}; | ||
|
|
||
| assertTrue(module.isEnabled()); | ||
| } | ||
|
|
||
| @Test | ||
| @WithConfig(key = "trace.test-kafka-module.enabled", value = "true") | ||
| @WithConfig(key = "data.streams.enabled", value = "true") | ||
| void testDataStreamsIsEnabledWhenBothEnabled() { | ||
| // When both super.isEnabled() and DSM are enabled, isEnabled() should return true. | ||
| InstrumenterModule.DataStreams module = | ||
| new InstrumenterModule.DataStreams("test-kafka-module") {}; | ||
|
|
||
| assertTrue(module.isEnabled()); | ||
| } | ||
|
|
||
| @Test | ||
| @WithConfig(key = "trace.test-kafka-module.enabled", value = "false") | ||
| @WithConfig(key = "data.streams.enabled", value = "false") | ||
| void testDataStreamsIsEnabledWhenBothDisabled() { | ||
| // When both super.isEnabled() and DSM are disabled, isEnabled() should return false. | ||
| InstrumenterModule.DataStreams module = | ||
| new InstrumenterModule.DataStreams("test-kafka-module") {}; | ||
|
|
||
| assertFalse(module.isEnabled()); | ||
| } | ||
| } |
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
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
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.
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.
Within this module, the DSM-only
USER_DROPguard is hand-copied 4 times: twice inTracingIterator.java(usingspanContext == null, no local-root check — see my comment there), once inKafkaConsumerInfoInstrumentation.java's poll span, and once inKafkaProducerInstrumentation.java's produce span (the latter two correctly usespan.getLocalRootSpan() == spanplus a comment explaining why). That the fixed/correct version only made it to 2 of 4 in-module copies is exactly the risk of this duplication. Cross-module sharing with kafka-streams-0.11 etc. is understandably not worth the trouble, but a small static helper here inKafkaDecorator(e.g.maybeForceDsmOnlyDrop(AgentSpan span)), or a private helper inTracingIteratorshared between its two call sites, seems low-cost and would have kept all four in-module copies in sync.