Remove unused DataStax Insights monitoring - #1035
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Team Run ID: 📒 Files selected for processing (12)
💤 Files with no reviewable changes (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe driver removes DataStax Insights clients, lifecycle listeners, schemas, helpers, tests, fixtures, and integration tests. The legacy monitor-reporting options remain for compatibility, are deprecated, default to Sequence Diagram(s)sequenceDiagram
participant SessionConfiguration
participant DefaultDriverContext
participant Loggers
participant LifecycleListeners
SessionConfiguration->>DefaultDriverContext: provide MONITOR_REPORTING_ENABLED
DefaultDriverContext->>Loggers: log warning when enabled or invalid
DefaultDriverContext->>LifecycleListeners: return empty listener list
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change removes unused monitoring behavior while preserving the deprecated configuration contract and existing configuration reporting. Validation passed, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 9 files. (1 skipped: 1 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a9b72c6 to
afda919
Compare
There was a problem hiding this comment.
Pull request overview
Removes DataStax Insights monitoring while preserving STARTUP metadata, driver-config reporting, and legacy configuration compatibility.
Changes:
- Removes the Insights runtime implementation, schemas, tests, and fixtures.
- Removes Insights-only dependency-report generation.
- Retains the monitoring option as a deprecated no-op with warning coverage.
Reviewed changes
Copilot reviewed 67 out of 67 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
changelog/README.md |
Records Insights removal. |
core/pom.xml |
Removes dependency-list generation. |
core/.../DseDriverOption.java |
Deprecates the legacy option. |
core/.../InsightsClientLifecycleListener.java |
Removes Insights lifecycle integration. |
core/.../insights/AddressFormatter.java |
Removes Insights address utility. |
core/.../insights/ConfigAntiPatternsFinder.java |
Removes configuration analysis. |
core/.../insights/DataCentersFinder.java |
Removes datacenter reporting. |
core/.../insights/ExecutionProfilesInfoFinder.java |
Removes profile reporting. |
core/.../insights/InsightsClient.java |
Removes the monitoring client. |
core/.../insights/InsightsSupportVerifier.java |
Removes DSE support detection. |
core/.../insights/PackageUtil.java |
Removes package metadata utility. |
core/.../insights/PlatformInfoFinder.java |
Removes platform reporting. |
core/.../insights/ReconnectionPolicyInfoFinder.java |
Removes policy reporting. |
core/.../insights/configuration/InsightsConfiguration.java |
Removes Insights configuration model. |
core/.../insights/exceptions/InsightEventFormatException.java |
Removes Insights exception. |
core/.../schema/AuthProviderType.java |
Removes auth schema. |
core/.../schema/Insight.java |
Removes event envelope schema. |
core/.../schema/InsightMetadata.java |
Removes metadata schema. |
core/.../schema/InsightType.java |
Removes event-type schema. |
core/.../schema/InsightsPlatformInfo.java |
Removes platform schema. |
core/.../schema/InsightsStartupData.java |
Removes startup report schema. |
core/.../schema/InsightsStatusData.java |
Removes status report schema. |
core/.../schema/LoadBalancingInfo.java |
Removes load-balancing schema. |
core/.../schema/PoolSizeByHostDistance.java |
Removes pool-size schema. |
core/.../schema/ReconnectionPolicyInfo.java |
Removes reconnection schema. |
core/.../schema/SSL.java |
Removes SSL schema. |
core/.../schema/SessionStateForNode.java |
Removes node-state schema. |
core/.../schema/SpecificExecutionProfile.java |
Removes profile schema. |
core/.../schema/SpeculativeExecutionInfo.java |
Removes execution schema. |
core/.../OptionsMap.java |
Defaults legacy monitoring to false. |
core/.../TypedDriverOption.java |
Deprecates typed legacy option. |
core/.../SessionBuilder.java |
Generalizes STARTUP metadata documentation. |
core/.../DefaultDriverContext.java |
Disables monitoring and warns on legacy enablement. |
core/.../InternalDriverContext.java |
Removes Insights-specific listener documentation. |
core/.../NoopDriverConfigReporter.java |
Removes obsolete Insights reference. |
core/.../StartupOptionsBuilder.java |
Generalizes metadata comments. |
core/src/main/resources/reference.conf |
Makes monitoring a disabled no-op. |
core/.../DependencyCheckTest.java |
Removes core dependency-report test. |
core/.../DependencyCheckTestBase.java |
Removes dependency-test infrastructure. |
core/.../insights/AddressFormatterTest.java |
Removes obsolete utility tests. |
core/.../insights/ConfigAntiPatternsFinderTest.java |
Removes obsolete analysis tests. |
core/.../insights/DataCentersFinderTest.java |
Removes obsolete finder tests. |
core/.../insights/ExecutionProfileMockUtil.java |
Removes Insights test helper. |
core/.../insights/ExecutionProfilesInfoFinderTest.java |
Removes obsolete profile tests. |
core/.../insights/InsightsClientTest.java |
Removes client unit tests. |
core/.../insights/InsightsSupportVerifierTest.java |
Removes support-detection tests. |
core/.../insights/PackageUtilTest.java |
Removes package utility tests. |
core/.../insights/PlatformInfoFinderTest.java |
Removes platform tests. |
core/.../insights/ReconnectionPolicyInfoFinderTest.java |
Removes policy tests. |
core/.../DefaultDriverContextTest.java |
Tests the legacy-option warning. |
core/src/test/resources/insights/duplicate-dependencies.txt |
Removes dependency fixture. |
core/src/test/resources/insights/malformed-pom.properties |
Removes malformed fixture. |
core/src/test/resources/insights/netty-dependency-optional.txt |
Removes optional dependency fixture. |
core/src/test/resources/insights/netty-dependency.txt |
Removes dependency fixture. |
core/src/test/resources/insights/ordered-dependencies.txt |
Removes ordering fixture. |
core/src/test/resources/insights/pom.properties |
Removes Maven fixture. |
core/src/test/resources/insights/test-dependencies.txt |
Removes dependency-list fixture. |
integration-tests/.../InsightsClientIT.java |
Removes Insights integration tests. |
integration-tests/.../DriverBlockHoundIntegrationCcmIT.java |
Removes obsolete monitoring comment. |
manual/core/integration/README.md |
Removes Insights Jackson usage. |
manual/core/non_blocking/README.md |
Removes monitoring guidance. |
mapper-processor/pom.xml |
Removes dependency-list generation. |
mapper-processor/.../DependencyCheckTest.java |
Removes dependency-report test. |
mapper-runtime/pom.xml |
Removes dependency-list generation. |
mapper-runtime/.../DependencyCheckTest.java |
Removes dependency-report test. |
query-builder/pom.xml |
Removes dependency-list generation. |
query-builder/.../DependencyCheckTest.java |
Removes dependency-report test. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
nikagra
left a comment
nikagra
left a comment
There was a problem hiding this comment.
[Minor] 🟡 manual/core/non_blocking/README.md:290 still lists * Automatic status reporting; under "NOT declared to be allowed" — the section explaining it is removed in this PR, and the monitor-reporting.enabled escape hatch it pointed at is now a no-op.
[Minor] 🟡 DependencyCheckTestBase was the only reader of /project.properties. Its four src/test/resources/project.properties files and the <testResources> filtering blocks in core, mapper-processor, mapper-runtime and query-builder poms are now dead.
| } | ||
| @SuppressWarnings("deprecation") | ||
| boolean insightsMonitoringRequested = | ||
| defaultProfile.getBoolean(DseDriverOption.MONITOR_REPORTING_ENABLED, false); |
There was a problem hiding this comment.
[Major] 🟠 Reading this in the constructor makes a bad value fatal: build() now throws ConfigException.WrongType where the old path (onSessionReady, wrapped by notifyListeners) only logged. A dead option shouldn't fail startup — wrap in try/catch.
|
|
||
| ### 4.19.2 | ||
|
|
||
| - [improvement] Remove unused DataStax Insights monitoring (#1027) |
There was a problem hiding this comment.
[Major] 🟠 4.19.2 is an upstream release that shipped without this; every other line here cites CASSJAVA/JAVA/PR. Fork-visible changes go in upgrade_guide/README.md, which has a 4.19.2.1 section and nothing for the enabled default flip to false.
|
|
||
| @Test | ||
| @SuppressWarnings("deprecation") | ||
| public void should_warn_when_removed_insights_monitoring_is_enabled() { |
There was a problem hiding this comment.
[Minor] 🟡 Only the true branch is covered. The default is now false — the path every session takes — so an inverted if would still pass. Add a case stubbing false and asserting verifyNoInteractions(logger.appender).
There was a problem hiding this comment.
Still open: the false path — the one every session takes — has no test. The two new cases cover true and a throwing profile, so reverting reference.conf to enabled = true, or dropping the , false default on DefaultDriverContext.java:279, keeps both green while every session in every deployment logs a WARN. A third case stubbing getBoolean(..., false) -> false and asserting verify(logger.appender, never()).doAppend(any()) pins it.
Separately, 🟢 verify(logger.appender) means times(1) and getValue() returns only the last event, so both cases would fail with a Mockito count error rather than a message assertion if any other constructor-time WARN is ever added. atLeastOnce() + getAllValues() is sturdier.
| # The default is true. | ||
| # | ||
| # Required: no (defaults to true) | ||
| # Required: no (defaults to false) |
There was a problem hiding this comment.
[Nit] 🟢 enabled is now the only option in the file with no description above # Required: — the prose moved above the block. reference/README.rst includes this file verbatim, so the published page shows it bare.
There was a problem hiding this comment.
Still open — enabled remains the only option in the file whose first comment is # Required:; the prose sits above the block, so a reader who lands on the option itself gets no signal it's dead. If the stanza stays (see my note on 1567 about dropping it outright), one line above # Required: is enough: # Whether to send DataStax Insights events. Ignored; Insights monitoring has been removed.
afda919 to
495ae84
Compare
| sessionName, | ||
| DseDriverOption.MONITOR_REPORTING_ENABLED.getPath()); | ||
| } | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
[Major] 🟠 The try spans the LOG.warn as well as the config read, so an appender failure gets misreported as "Could not read deprecated configuration option" — it read it fine. Worse, the recovery path is itself unguarded: Loggers.warnWithException calls warn again, throws again, and nothing catches it, so it escapes the DefaultDriverContext constructor and fails SessionBuilder.build(). The constructor did no logging at all before this PR, so that's a new failure mode. Narrowing the try to just the getBoolean on line 279 keeps the round-1 fix and drops this.
| "[{}] Configuration option {} is deprecated and ignored; " | ||
| + "DataStax Insights monitoring is no longer supported", | ||
| sessionName, | ||
| DseDriverOption.MONITOR_REPORTING_ENABLED.getPath()); |
There was a problem hiding this comment.
[Minor] 🟡 @SuppressWarnings("deprecation") on line 277 covers only the variable declaration, so lines 285 and 292 reference MONITOR_REPORTING_ENABLED outside its scope. The root pom sets failOnWarning=true; the build survives today only because showDeprecation is left at its default, so these turn into hard errors the moment anyone enables it, adds -Xlint:deprecation, or runs CI with deprecation lint on. This file already solves it three times (lines 832-833, 881-882, 925-926) — hoist a suppressed local and use it at all three sites:
@SuppressWarnings("deprecation")
DriverOption legacyMonitorReporting = DseDriverOption.MONITOR_REPORTING_ENABLED;| } | ||
| return Collections.emptyList(); | ||
| } | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
[Question] 🔵 This now returns exactly what InternalDriverContext.getLifecycleListeners()'s own default returns (InternalDriverContext.java:197), yet it's still routed through a LazyReference registered with the cycleDetector (line 242) and a getLifecycleListeners() override (line 1280) — a volatile read and a cycle-detector entry for a compile-time constant. Is the protected hook deliberately kept for third-party subclasses (removing it would be a source break)? If so, a comment saying that would help; if not, the lazy ref and the override can both go.
| throw new InvalidObjectException("Proxy required"); | ||
| } | ||
|
|
||
| @SuppressWarnings("deprecation") |
There was a problem hiding this comment.
[Minor] 🟡 A method-wide suppression to cover one line silences deprecation warnings across all 157 lines of the driver's default table — the one method whose job is to enumerate supported defaults, and so the one place a newly deprecated option should be noticed. @SuppressWarnings can't annotate a bare statement, so the narrow form is a suppressed local, same shape as DefaultDriverContext.java:832-833:
@SuppressWarnings("deprecation")
TypedDriverOption<Boolean> legacyMonitorReporting = TypedDriverOption.MONITOR_REPORTING_ENABLED;
map.put(legacyMonitorReporting, false);| # Modifiable at runtime: no | ||
| # Overridable in a profile: no | ||
| enabled = true | ||
| enabled = false |
There was a problem hiding this comment.
[Minor] 🟡 This default now lives in three places: here, OptionsMap.java:318, and the literal false at DefaultDriverContext.java:279. Edit one and they disagree silently — HOCON users get the code default, OptionsMap users get the map value, and the paths can't be told apart. Since the code already carries a hard default, dropping this stanza entirely is the cleaner deprecation, and it keeps a dead option off the published manual/core/configuration/reference/README.rst page, which renders this file verbatim.
| #### DataStax Insights monitoring has been removed | ||
|
|
||
| The driver no longer sends DataStax Insights startup or status events. The legacy | ||
| `advanced.monitor-reporting.enabled` option and its public constants remain deprecated no-ops for |
There was a problem hiding this comment.
[Minor] 🟡 "remain deprecated no-ops" isn't accurate. DseDriverOption.MONITOR_REPORTING_ENABLED still resolves the live path advanced.monitor-reporting.enabled, and reading it is exactly what produces the new warning; the TypedDriverOption constant still reads and writes a real config key. The entry also omits the one action a reader needs — someone who leaves enabled = true in application.conf gets a WARN on every session creation for the rest of the release line and nothing here tells them to delete the line. Suggest: the option is still readable but has no effect, and setting it to true logs a warning until it's removed from the configuration.
Closes #1027. Part of #1025.
What and why
Remove the DataStax Insights client, lifecycle listener, schemas, monitoring option, tests, fixtures, and generated dependency-report files. ScyllaDB does not use or test Insights, so this removes background production behavior and test/build code that the team otherwise has to support without coverage.
Client/application STARTUP metadata and Scylla driver-config reporting are preserved because they have non-Insights consumers.
Validation
mvn -pl core,query-builder,mapper-runtime,mapper-processor clean test— 4,571 tests, no failures or errors.mvn -pl integration-tests -am -DskipTests test-compileDependencies: none. Cherry-picked commits: none.