Skip to content

fix(cost): close RequestCost instrumentation gaps and re-base the price table - #36978

Merged
wezell merged 11 commits into
mainfrom
issue-36977-requestcost-pricing
Aug 11, 2026
Merged

fix(cost): close RequestCost instrumentation gaps and re-base the price table#36978
wezell merged 11 commits into
mainfrom
issue-36977-requestcost-pricing

Conversation

@wezell

@wezell wezell commented Aug 10, 2026

Copy link
Copy Markdown
Member

Fixes #36977

Why

@RequestCost scores request heaviness so usage can be metered and rate-limited. Auditing where the annotation actually sits against Glowroot main-thread profiles from six production instances found the instrumentation concentrated on the Contentlet API while the traffic was somewhere else — and one whole class of cost being discarded.

Instance shape Where the time actually went Metered before?
write-heavy API addContentToIndex — ~10% of all samples, blocking no
GraphQL 60% / VTL 31% prerender park 36%, NavTool.getNav 11% no
Velocity + remote API CircuitBreakerUrl outbound HTTP ~50% yes, priced 4
GraphQL 54% GraphQL execute, transform, relationship N+1 no
traditional Velocity filter chain, render, binaries partly

A GraphQL-only customer could saturate a node and accrue almost nothing.

What changed

Metering gaps closed

Where Why it mattered
ESContentFactoryImpl.findContentlets(List) The bulk loader behind every search result — GraphQL, /api/content, /api/es/search, page render. It bypasses find(), so nothing metered it. Now per contentlet: base fee, plus a surcharge on rows read from the DB
incrementCost background path Returned before the totals when no request was on the thread. Reindexing, scheduled publishing, remote publishing and content indexing were priced correctly and then thrown away
enterprise.priv.ESSearchAPIImpl.esSearchRaw /api/es/search bypassed the existing ES_QUERY charge entirely
VTLResource.evalVelocity /api/vtl/* called the engine directly, bypassing the annotated merge
DotDirective.render Velocity rendering was not metered anywhere on the page path — neither #dotParse / #parseContainer nor the top-level page merge, since all three mode handlers call Template.merge() directly rather than an annotated VelocityUtil method. (What each container fetched was metered, via find() and cachedIndexSearch; the work of assembling the page was not.) Charged past the getFromCache() short-circuit, so cacheable containers stay cheap
VelocityLiveMode / EditMode / PreviewMode The page's own template merge was unmetered too. Charged at the merge site, not on serve() — LiveMode on writePage(), which the two page-cache hit paths return before reaching, so a cached page pays no merge
PreRenderSEOWebAPIImpl Own HttpClient, so CircuitBreakerUrl's charge never fired
ContentletIndexAPIImpl, NavTool, DotGraphQLHttpServlet New CONTENT_INDEX, NAV_BUILD, GRAPHQL_QUERY
ESContentletAPIImpl terminals CONTENT_CHECKIN / CHECKOUT / COPY / MOVE existed in the enum with nothing charging them

Pricing model. Meter customer-visible operations, with the line drawn at what the customer can influence:

Whose? Priced?
Did this need the database at all? theirs — cacheable containers/pages, TTLs, query shape yes
How many statements, what batch size, which plan? ours no

Content is CONTENT_FROM_CACHE(1) per contentlet asked for plus CONTENT_FROM_DB(10) on the ones read from Postgres — 1 warm, 11 cold, an 11× incentive to cache. The surcharge is per missed row, not per SQL statement; the 200-row batching is our detail. There is deliberately no generic DB_QUERY.

Price table re-based on resource-time, then the DB tier compressed — what is metered is capacity consumed on this node, not wall time. A query parks the thread and burns cycles on Postgres. At DB_QUERY 25 the table said DB was 50% of a page render; the profile said template rendering dominated. At 10 it reads merges 59% / DB 31% / ES 10%, which matches.

IMAGE_FILTER_TRANSFORM was 2 and FILE_METADATA_GENERATE was 3 — decoding an image or running Tika burns a core and a large buffer. Both now 50.

Reporting. REQUEST_COST_DENOMINATOR 1 → 10 keeps reported tokens in their historical range. Header and snapshot totals are rounded — both have only ever carried whole numbers, and a collector parsing them as ints would break on a fraction. Per-request averages stay fractional, as always. Rate-limit defaults scaled with the table (still off by default).

Collector change required

RequestCostSnapshot gains windowJobTokens and lifetimeJobTokens (doubles), reported separatelywindowTokens stays request-only, total consumption is the sum. Unknown fields are dropped silently by the ingestor, so this is safe to deploy first, but background cost is not billed until those two columns exist.

Expect a step change in reported tokens at deploy: prices re-based, denominator moved. Roughly range-preserving for requests, but per-endpoint series shift by different amounts.

Testing

RequestCostSnapshotTest and RequestCostPublisherTest updated for the two new fields, including the "exactly N fields on the wire" guard.

Verified with javac against the built classpath — error count identical to the baseline. The full Maven build fails in dotcms-core-web's nx step, pre-existing and unrelated.

Not validated against a running instance. Prices are order-of-magnitude estimates from sampled time-share; placements were verified by reading call chains, not by watching a request. The calibration loop — run a week, compare per-endpoint mean response time against mean cost, investigate outlier ratios — is what turns these into measured numbers.

Known gaps, deliberately out of scope

  • Embedding generation / vector DB indexing is not priced at all — no constant, nothing annotated. Likely the largest remaining gap given inference cost.
  • Job cost is one undifferentiated bucket, not split by job type.
  • The telemetry publisher self-charges HTTP_FETCH for its own push (~10 tokens/window).
  • Hibernate is invisible to DB-level metering — PermissionBitAPIImpl.getPermissionsByRole goes through net.sf.hibernate.loader.Loader, never DotConnect.

Reviewer notes

Three traps worth knowing if you touch this:

  1. The enum is not an index of what is metered. ES_QUERY, ES_COUNT, WORKFLOW_ACTION_RUN and IMAGE_FILTER_TRANSFORM are charged with direct incrementCost(...) calls, invisible to a @RequestCost grep. Grep both forms.
  2. ByteBuddy weaves bytecode, so self-invocation fires the advice — unlike CDI proxies. checkin has 12 overloads chaining into each other; annotating two in one chain double-charges. Every placement here targets a verified terminal.
  3. Annotations are not inherited. A @RequestCost on an interface default does nothing for an override — this had already left CONTENT_FROM_DB dead on the common single-find path.

Docs ship with the change: docs/requestcost-session-summary.md (narrative) and docs/requestcost-placement-analysis.md (evidence and backlog). Tenant identities are anonymised.

Defects found during the audit, filed separately: #36970, #36971.

🤖 Generated with Claude Code

This PR fixes: #36977

…ce table (#36977)

Audited @RequestCost placement against Glowroot production profiles from six
tenant instances. The instrumentation was concentrated on the Contentlet API
while the traffic was elsewhere: a GraphQL-only customer could saturate a node
and accrue almost nothing.

Metering gaps closed:
- findContentlets(List) - the bulk loader behind every search result (GraphQL,
  /api/content, /api/es/search, page render). Bypassed find(), so it was
  entirely uncosted. Now charged per contentlet, cache hit vs DB miss.
- Background work no longer discarded. incrementCost returned before the totals
  when no request was on the thread, so reindexing, scheduled publishing,
  remote publishing and content indexing were priced correctly and then thrown
  away. Reported as windowJobTokens / lifetimeJobTokens.
- /api/es/search bypassed ES_QUERY via enterprise ESSearchAPIImpl.
- /api/vtl/* bypassed VELOCITY_MERGE by calling the engine directly.
- #dotParse / #parseContainer were free, so a 30-container page priced as one.
- Prerender used its own HttpClient, so HTTP_FETCH never fired.
- CONTENT_INDEX, NAV_BUILD and GRAPHQL_QUERY added; CHECKIN/CHECKOUT/COPY/MOVE
  had prices in the enum that nothing charged.

Price table re-based on resource-time, then the DB tier compressed: what is
metered is capacity consumed on this node, not wall time. Cache-hit vs miss is
priced because customers control caching; statements-per-miss is not, because
they cannot. IMAGE_FILTER_TRANSFORM and FILE_METADATA_GENERATE were priced
below a cache read and are now at the top of the scale.

Reporting keeps its historical range via REQUEST_COST_DENOMINATOR 1 -> 10, and
token totals stay integral so a collector parsing them as ints still works.

Requires two new ingestor columns before background cost is billed.
@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Claude finished @wezell's task in 4m 28s —— View job


Code Review

  • Read the full diff (23 files)
  • Analyzed cost/metering logic (double-charge, replay, error paths)
  • Checked pricing table & snapshot changes
  • Verified call chains (move, checkin, addContentToIndex, find/findContentlets)

I reviewed the diff (b185cde4..HEAD, 23 files, +387/-65). This is a well-reasoned change and I found no blocking issues. Call-chain terminals were chosen correctly and I could not find a double-charge:

  • move(...) overloads (479→501→588) — only the terminal at :588 carries @RequestCost, so self-invocation fires once. ✅
  • addContentToIndex(Contentlet)addContentToIndex(List) — only the List overload is annotated, single charge. ✅
  • Single find() uses contentFactory.find()findInDb, not findContentlets(List), so the CONTENT_FROM_CACHE base fee isn't double-billed on the scalar path. ✅
  • Removed DB_QUERY enum constant has no remaining code references (only comments). ✅
  • Snapshot field ordering matches the publisher call args (windowJobTokens, lifetimeJobTokens) and the log format string has 9 specifiers / 9 args with matching types. ✅
  • Error paths: RequestCostHandler.incrementCost wraps the direct call in try/catch with warnAndDebug, correctly matching the annotation path's suppress = Throwable.class protection so metering can't break serving. ✅

New Issues

  • 🟡 Medium: dotCMS/src/main/java/com/dotcms/cost/RequestCostApiImpl.java:294 — The core new behaviors have no unit coverage. incrementCost(..., times) (the price * times multiplication and the times <= 0 no-op) and the background request == nulljobCostForWindow/jobCostTotal accumulation are the substance of this PR, but the only test change is RequestCostSnapshot serialization (field presence/round-trip). Nothing asserts that a background charge lands in the job counters instead of vanishing, that times scales linearly, or that the header floors at 1.00 for a sub-denominator request (:395). Given the PR is explicitly "not validated against a running instance," a small unit test around RequestCostApiImpl would catch a regression in exactly the logic that was just rewired. Fix this →

  • 🟡 Medium: dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java:5730internalCheckin is now @RequestCost(CONTENT_CHECKIN) (50). A checkin also reindexes via addContentToIndex(...) (the sync-commit listener), which is @RequestCost(CONTENT_INDEX) (25), so a single checkin now bills 75+. This is the same intentional stacking documented on move() at :580, and is defensible (checkin genuinely reindexes) — but unlike move it isn't called out. What to verify: confirm this stacking is intended for checkin, and if so add the same one-line note you put on move so the next reader doesn't "fix" it as a double-charge. (Not a bug — flagging only because the two parallel cases are documented inconsistently.)

Non-blocking

  • The reviewer (@sfreudenthaler) already asked to trim the verbose comments; that's a judgment call for you, but I'll note the density is high (e.g. the multi-paragraph block comments in RequestPrices.java and RequestCostApiImpl.java). Not a correctness concern.
  • CircuitBreakerUrl.java:78 — the only change is an indentation shift on the Lazy.of(...) line (extra leading spaces), no behavioral change. Looks like an accidental whitespace edit worth reverting to keep the diff tight.

Nice touches worth keeping: the times <= 0 guard, the header floor-at-1 with the "genuinely zero-cost only" carve-out, and the sentinel-aware i.equals(contentlet.getInode()) cache-hit check all read correctly.

· branch issue-36977-requestcost-pricing

wezell added 4 commits August 10, 2026 13:21
…36977)

Velocity rendering was not metered anywhere on the page path. All three mode
handlers call Template.merge() directly rather than an annotated VelocityUtil
method, so neither the page nor its nested directives charged a merge - the
annotated VelocityUtil.merge/mergeTemplate/eval only ever fired for AI prompt
evaluation and SAML relay state.

Charged at the merge itself rather than on serve():

- LiveMode: on writePage(), which the two page-cache hit paths return before
  reaching - so a cached page correctly pays no merge.
- Edit/Preview: immediately before the merge call. serve() also does permission
  checks and context building, and would have billed a merge that never ran.

Nested #dotParse / #parseContainer continue to charge separately in
DotDirective.render, past its own cache short-circuit.
…ntlets (#36977)

The cache lookup already visits every inode and already knows which ones
missed, but the misses were re-derived afterwards with
CollectionUtils.subtract(inodes, conMap.keySet()) - which builds a HashBag of
the hit keys, walks the inodes against it, and returns a list that was then
copied into a second ArrayList. Two passes and three allocations to recompute
something the first loop knew.

Also fixes a latent duplicate-handling wart: subtract has multiset semantics,
so a repeated inode that WAS in cache would fall out of the bag on its second
occurrence and be queried from the database anyway. Filtering on the cache
result cannot do that.

Behaviour is otherwise identical for the normal case where an inode maps to a
contentlet with the same inode.
Reverts the push URL default to null. isEnabled() gates on url AND token, so a
default URL could not activate the publisher on its own - but it meant anyone
who set only REQUEST_COST_PUSH_TOKEN would silently start posting usage data to
a dotcms.dev endpoint they never named. The collector has to be opted into
explicitly.

The collection interval default stays at 300s (up from 60s): a five-minute
window is enough resolution for billing and cuts the push volume fivefold.
…ocument stacking (#36977)

Three findings from the automated review, all valid.

1. The four direct incrementCost(..) calls ran unguarded on page-render and
   content-load paths. The annotation path is protected - RequestCostAdvice.enter
   is @Advice.OnMethodEnter(suppress = Throwable.class) - so a failure in the
   cost API can never break the metered method there. The direct calls had no
   such protection: a transient locator or init failure would have propagated
   out of DotDirective.render and ESContentFactoryImpl.findContentlets and
   broken the page or the search result. Metering must never take down serving.
   Routed all four through RequestCostHandler, which already wraps exactly this
   for the advice and CDI paths, rather than scattering try/catch.

2. The x-dotrequest-cost header rounded to "0.00" for any request costing less
   than half the denominator - at the default of 10, every request reading a
   single warm contentlet. A request that did real work reported as free. Now
   floored at 1 whenever the raw cost is non-zero; only a genuinely zero-cost
   request reports 0.00. Window and lifetime totals are unaffected, they sum raw
   units and divide once.

3. CONTENT_MOVE and CONTENT_COPY are not terminals and stack with CHECKIN and
   INDEX. That is deliberate - a copy really does check in each version and a
   move really does reindex - and is not the double-charge trap, which is about
   one operation counted twice rather than two operations each counted once.
   Documented on both so a future reader does not "fix" it.
@wezell
wezell enabled auto-merge August 10, 2026 19:08

@sfreudenthaler sfreudenthaler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wezell i’m good with the changes. I’d like to see the comments ripped out or much less verbose in most cases, but not blocking a merge on it

Comment thread docs/requestcost-placement-analysis.md Outdated
Comment thread docs/requestcost-session-summary.md Outdated
Comment thread dotCMS/src/main/java/com/dotcms/cost/LeakyTokenBucketImpl.java Outdated
wezell and others added 3 commits August 10, 2026 20:13
Co-authored-by: Steve Freudenthaler <31257998+sfreudenthaler@users.noreply.github.com>
@github-actions github-actions Bot removed the Area : Documentation PR changes documentation files label Aug 11, 2026
wezell and others added 2 commits August 10, 2026 21:53
)

The report template renders the total as requestCost / REQUEST_COST_DENOMINATOR,
whose default moved from 1.0 to 10.0 when the price table was re-based. Compute
the expected value the same way the report does so the test holds under any
configured denominator.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VTt9ymEwfmHV9DSKe1vPuJ
…otal (#36977)

RequestCostApiImpl only reads REQUEST_COST_DENOMINATOR in @PostConstruct, so the
test's hand-constructed instance keeps the 1.0 field default while the report
divides by the CDI singleton's 10.0 — the expected and rendered totals diverged.
Compute the expectation with the same instance the report uses.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VTt9ymEwfmHV9DSKe1vPuJ
…lets (#36977)

The single-pass rewrite of the cache-miss collection marked any non-null cache
entry as found, but the cache stores the CACHE_404_CONTENTLET sentinel under the
requested inode after a failed single-item lookup. A sentinel hit skipped the DB
fetch and the final keyed lookup then dropped the inode from the result entirely
(ESContentFactoryImplTest.findContentlets: expected 991, got 988). Count a hit
only when the cached inode equals the requested one, matching the old
CollectionUtils.subtract semantics.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VTt9ymEwfmHV9DSKe1vPuJ
@wezell
wezell added this pull request to the merge queue Aug 11, 2026
Merged via the queue into main with commit 4d814eb Aug 11, 2026
67 checks passed
@wezell
wezell deleted the issue-36977-requestcost-pricing branch August 11, 2026 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Fix RequestCost instrumentation gaps and re-base the price table on resource-time

2 participants