spec(experiments): CAEM-backed experiment goal result queries

[freddyDOTCMS]

Important

Design Artifacts: View Spec & Implementation Details

Summary

• Adds spec.md for issue Add CAEM-backed experiment result query classes with legacy CubeJS fallback switch #37227 — CAEM-backed result query implementations (bounce rate, exit rate, reach-target, URL-param) that call the CAEM analytics API instead of CubeJS
• Both implementation paths coexist; a runtime configuration switch (DOT_CAEM_ENABLED) selects between them with CubeJS as the safe default
• Spec covers 5 user stories (3× P1, 2× P2), 17 FRs, 7 success criteria, and Legacy Considerations

Closes #37227
Depends on: #37223 (CAEM sessions endpoint), #37224 (CAEM sessions behavior endpoint)

Review checklist

• User stories are independently testable and correctly prioritized
• Functional requirements are testable and unambiguous
• Legacy Considerations accurately describe the backward-compatibility guarantee
• Assumptions are valid given current CAEM pipeline state

Test plan

• This PR carries spec.md only — no implementation code
• Reviewer approves the spec before /speckit-plan runs on the feature branch

🤖 Generated with Claude Code

[claude]

Claude finished @freddyDOTCMS's task in 1m 43s —— View job

---

Spec Review — CAEM-backed experiment goal result queries

• Read spec.md
• Verified referenced classes, interface, factory, and goal types against the codebase
• Posted review

This PR carries spec.md only, so there is no executable code to review. I verified the spec's claims against the real code it will touch. The spec is well-structured and mostly accurate — the class names, package (com.dotcms.experiments.business.result), goal types (BOUNCE_RATE, EXIT_RATE, REACH_PAGE, URL_PARAMETER, CLICK_ON_ELEMENT), and the fact that URL_PARAMETER currently reuses ReachTargetAfterExperimentPageResultQuery (ExperimentResultsQueryFactory.java:73) are all correct. Two design assumptions in the spec are in tension with how the current code is actually wired — worth resolving before /speckit-plan so the plan targets the right seam.

New Issues

• 🟡 Medium: specs/37227-caem-experiment-goal-queries/spec.md:136-137 (Key Entities) — The switch cannot live in ExperimentResultsQueryFactory / MetricExperimentResultsQuery as described. That interface returns a CubeJSQuery (MetricExperimentResultsQuery.java:18), and the factory's create() / createWithDayGranularity() produce a CubeJSQuery that ExperimentsAPIImpl.getSummary()/getTotalSessions() send via cubeClient.send(...), yielding a CubeJSResultSet that flows into the private getResults(CubeJSResultSet, …) and then Bayesian (ExperimentsAPIImpl.java:1430-1441, 1311). A CAEM-backed class making HTTP calls to CAEM cannot naturally "implement this same interface" (it has no CubeJSQuery to return), and even if it did, the downstream pipeline consumes a CubeJSResultSet, not a neutral result model. The real dispatch seam is one level up — getSummary/getTotalSessions in ExperimentsAPIImpl — not the query factory. Recommend the spec either introduce a new abstraction (e.g. a ResultQueryExecutor returning the neutral ExperimentResults/VariantResults model) or explicitly name ExperimentsAPIImpl as the switch point. As written, FR-013 and the "call sites unchanged" claim in Legacy Considerations (spec.md:161) understate the scope.

• 🟡 Medium: specs/37227-caem-experiment-goal-queries/spec.md:123,151 (FR-008 / SC-003 / US5 scenario 3) — "Toggle at runtime without a restart" conflicts with the factory's cached dispatch map. ExperimentResultsQueryFactory holds experimentResultQueryHelpers in a Lazy.of(() -> createHelpersMap()) (ExperimentResultsQueryFactory.java:64-65) that is computed once and never recomputed. If the CAEM/CubeJS selection is resolved at map-construction time, flipping DOT_CAEM_ENABLED will not take effect until a restart, violating SC-003 and US5 scenario 3. The plan must read the config on each evaluation (per-call dispatch), not at initialization. Worth calling out in the spec so this isn't discovered late.

• 🟡 Medium: specs/37227-caem-experiment-goal-queries/spec.md:35,106,108,132 — "Surface the failure" is asserted repeatedly but never defined. Edge cases and FR-017 require CAEM errors (auth failure, non-2xx, malformed body, unreachable endpoint) to be "surfaced rather than silently swallowed," but the spec never states how — thrown exception vs. error field on the result vs. logged-and-empty. The current CubeJS path returns a populated ExperimentResults; consumers (admin UI, results API, Bayesian calc at ExperimentsAPIImpl.java:1418) have no defined behavior for a hard failure. Recommend one FR pinning the observable failure contract (exception type / HTTP status returned to the API caller) so SC-004's "error surfacing" is testable rather than subjective.

Notes (non-blocking)

• spec.md:107 vs spec.md:110 — an unrecognized runningId is specified to return empty/zero, while a missing experimentId/runningId invariant is an error. These are consistent, but both depend on the external CAEM contract (Extend GET /v1/analytics/sessions with bounce/exit metrics and variant dimension #37223/Add GET /v1/analytics/sessions/behavior endpoint with reach-target and URL-param goal support #37224). Assumption is correctly flagged; just confirm the "unrecognized runningId ⇒ empty, not error" behavior is actually guaranteed by CAEM before relying on it in FR-014.
• FR-010 accurately lists the three concrete CubeJS classes (no separate URLParameterResultQuery exists — it reuses ReachTargetAfterExperimentPageResultQuery). Good.

Overall: solid spec, no blocking issues. The two design-seam findings above are the main things to reconcile before planning, since they change where the code lands.
· issue-37227-add-caem-backed-experiment-result-query-classes-with-legacy-cubejs-fallback-switch

[erickgonzalez]

Verdict

REQUEST CHANGES — 5 blockers.

The spec assumes a switch seam that the current interface cannot support, declares unit tests only with no justification for the other types, and leaves both the config key and the authorization of the new path undefined.

Blockers

B1 — [CRITICAL] Unit tests only, other types not justified

• FR-015/016/017 and SC-004 declare unit tests exclusively. Principle V requires the layer-appropriate type(s), or an explicit statement from the developer as to why one cannot be implemented ("silence is not consent"). There is live coverage this change touches: dotcms-integration/src/test/java/com/dotcms/experiments/business/ExperimentResultsQueryFactoryIntegrationTest.java:93 asserts exactly the query the switch replaces.
• Add FRs for integration tests (dispatch with flag on/off + ExperimentResults parity) and Postman against the results endpoint, or one explicit line per omitted type with its reason.

B2a — [CRITICAL] The existing interface returns CubeJSQuery

• Key Entities states the CAEM classes "implement this same interface", but dotCMS/src/main/java/com/dotcms/experiments/business/result/MetricExperimentResultsQuery.java:18 declares CubeJSQuery getCubeJSQuery(Experiment). A CAEM implementation cannot return a CubeJSQuery.
• Define the real seam (a provider-agnostic abstraction) and correct FR-001/FR-010 and Key Entities accordingly.

B2b — [CRITICAL] The seam does not fit in the factory, and call sites do change

• Legacy Considerations claims the ExperimentsAPIImpl call sites stay "unchanged", but ExperimentsAPIImpl.java:1429-1440 return CubeJSResultSet via CubeJSClient, and the mapping to VariantResults/GoalResults that FR-013 attributes to the query classes actually happens in ExperimentsAPIImpl.java:1324-1345, reading Events.* keys.
• Rewrite FR-008/FR-013 and Legacy Considerations to place the switch where ExperimentResults is produced, not where the query is built.

B3 — [HIGH] Configuration key undefined

• FR-008 and Key Entities say "a dotCMS Config property" without naming the key; the PR body names DOT_CAEM_ENABLED. The convention in this module is a constant in dotCMS/src/main/java/com/dotcms/featureflag/FeatureFlagName.java:10 read via Config.getBooleanProperty (ConfigExperimentUtil.java:113). Also, SC-003 (toggle without restart) conflicts with the static Lazy that caches the dispatch map in ExperimentResultsQueryFactory.java:64.
• Name the key and its default (false), and require in an FR that it be read per evaluation or via SystemTableUpdatedKeyEvent as in ConfigExperimentUtil.java:79-85.

B4 — [HIGH] Authorization and credential scope not expressed

• FR-012 says it reuses the EventAnalyticsProxyHelper token, but buildAuthHeader(Host) is package-private and site-scoped (EventAnalyticsProxyHelper.java:275), and with no token the request goes out without an Authorization header (lines 283-291). The CubeJS path is user+site-scoped via resolveAnalyticsApp(user) -> getCurrentHost() (AnalyticsHelper.java:466); no FR carries user/site into the CAEM path.
• Add FRs: preserve the same authorization as the CubeJS path, fail (do not call) when no token is available, and never log the token or a URL containing it.

B5 — [HIGH] SC-007 asserts behavior that does not exist today

• SC-007 says CLICK_ON_ELEMENT "continues to use the CubeJS path", but the map in ExperimentResultsQueryFactory.java:69-74 has no entry for that type, so getMetricCubeJSQuery (lines 163-165) NPEs today. The criterion is unverifiable as written.
• Reword SC-007 as "no change to current behavior for unsupported types", or take it out of scope.

Questions for the author

Q1 — Where does the seam live: at the query-builder level, or an ExperimentResultsProvider that returns a complete ExperimentResults? Today the factory produces a query and the parsing lives in ExperimentsAPIImpl.getResults. My recommendation: a provider at the getResults level, leaving MetricExperimentResultsQuery and the three CubeJS classes untouched. Answering this resolves B2a and B2b, and rewrites FR-010/FR-013.

Q2 — Which Host resolves the CAEM base URL and token, given that getResults(experiment, user) has no request context? CubeJS uses getCurrentHost(), which does not exist in the experiment-finalization job (ExperimentsAPIImpl.java:1443). Suggested: derive the Host from the experiment's page and pass it explicitly. Resolves B4.

Q3 — Is the daily series in scope? getSummary uses createWithDayGranularity with timeDimension("Events.day") (ExperimentResultsQueryFactory.java:90-92) and the summarize loop consumes it. No FR mentions it: if CAEM does not return day granularity, the shape of ExperimentResults changes. Resolves part of B2b.

Q4 — Does URL_PARAMETER use paramName/paramValue (FR-006), or the single QUERY_PARAMETER with a regex parameter name declared in MetricType.java:41-45? Today that type reuses ReachTargetAfterExperimentPageResultQuery (ExperimentResultsQueryFactory.java:73). This determines the parameters sent to CAEM.

Recommended next steps

1. Run /speckit-clarify for Q1-Q4 before anything else; do not plan yet — Q1 and Q3 determine the abstraction and the shape of ExperimentResults, so a plan written first gets redone in full.
2. Rewrite FR-001, FR-008, FR-010, FR-013 and Key Entities (MetricExperimentResultsQuery, ExperimentResultsQueryFactory) per the answer to Q1 — resolves B2a and B2b.
3. Name the config key, its default, and its re-read semantics in FR-008; add the proposed constant to FeatureFlagName — resolves B3.
4. Add security FRs: authorization equivalent to the CubeJS path, explicit failure with no token, and a prohibition on logging the token — resolves B4.
5. Add integration and Postman FRs (or the explicit per-type justification) and reword SC-004 — resolves B1; reword SC-007 — resolves B5.
6. Convert the Edge Cases section from interrogative prose into Given/When/Then scenarios inside the story that owns each case; move the HTTP client choice and the response-envelope parsing detail to plan.md.
7. Reorder US5 to ship first (US1-US4 are not independently testable without the flag), and move the physical-design claims in Legacy Considerations down to plan.md.
8. Chain /speckit-clarify -> spec re-review -> /speckit-plan -> /speckit-tasks -> /speckit-analyze, and only then /speckit-implement.