Skip to content

[api][java][python] Record embedding token usage metrics - #1047

Open
hulincup wants to merge 2 commits into
apache:mainfrom
hulincup:feat/issue-858-embedding-token-metrics
Open

[api][java][python] Record embedding token usage metrics#1047
hulincup wants to merge 2 commits into
apache:mainfrom
hulincup:feat/issue-858-embedding-token-metrics

Conversation

@hulincup

Copy link
Copy Markdown
Contributor

Linked issue: #858

Purpose of change

Embedding providers already report token usage (Bedrock on Java; OpenAI/Tongyi via the Python cross-language bridge), and the types to carry it (EmbeddingTokenUsage / EmbeddingResult.tokenUsage) already exist. But nothing reads it back to record metrics, so provider usage is dropped before it reaches the metric layer. The chat side already records promptTokens/completionTokens; embeddings had no equivalent, which made embedding cost/usage hard to validate — exactly the gap #858 calls out.

This mirrors the chat path:

  • Java BaseEmbeddingModelSetup.recordTokenMetrics(modelName, promptTokens, totalTokens) records under the same model key-value group used by chat metrics, called from both embedWithUsage overloads (the chokepoint covering direct calls, vector stores, and RAG).
  • Python BaseEmbeddingModelSetup._record_token_metrics / _record_token_usage does the same in embed_with_usage, keeping Java/Python parity (the chat side already records in Python via _record_token_metrics).

Two design notes:

  • Embeddings record totalTokens in place of chat's completionTokens, since there is no completion.
  • Embedding calls do not run inside a plan action (unlike chat, whose recording happens in ChatModelAction with a request-scoped group). Vector-store, RAG, and direct calls reach the setup directly, so the resource-bound metric group injected via setMetricGroup is used instead. This is the one deliberate asymmetry with the chat path and is documented on the method.

No change to RowTypeInfo handling, no payload change for a schema that renders, and embed (non-usage) methods are unchanged — they discard usage because it is not returned (now noted in their Javadoc).

Tests

Mirrors BaseChatModelSetupTokenMetricsTest for both languages:

  • Java: records under model group, batch path, no-op when no metric group bound, no-op when provider reports no usage, null/blank model name records nothing and is rejected by recordTokenMetrics, counters accumulate.
  • Python: same matrix via a _MockMetricGroup, plus batch and accumulation.

Verification:

  • Python: pytest flink_agents/api/embedding_models/tests/ — 10 passed (4 existing + 6 new), ruff check and ruff format --check clean.
  • Java: cannot build locally (Java 11 required, host is Java 8). Verification relies on CI (./tools/ut.sh); spotless/line-length checked manually against google-java-format AOSP 100-col rules.

API

Yes. New public method BaseEmbeddingModelSetup.recordTokenMetrics(String, long, long) (Java) and _record_token_metrics / _record_token_usage (Python, protected). No existing signature changes. New counter names promptTokens / totalTokens under the existing model group, consistent with chat's promptTokens (chat also emits completionTokens; embeddings do not).

Documentation

  • doc-not-needed — adds metric counters under the existing model group with names aligned to the chat side; no public API surface or config changes beyond the new recording method.

Was this patch authored or co-authored using generative AI tooling?

  • Yes

Generated-by: Claude Code 2.1.220 (glm-5.2[1m])

Embedding providers (Bedrock on Java; OpenAI/Tongyi via the Python
cross-language bridge) already populate EmbeddingTokenUsage on the
returned EmbeddingResult, but nothing reads it back to record metrics,
so provider usage is dropped before it reaches the metric layer. The
chat side already records promptTokens/completionTokens; embeddings
had no equivalent.

Mirror the chat path: add recordTokenMetrics to BaseEmbeddingModelSetup
(Java) and _record_token_metrics to the Python BaseEmbeddingModelSetup,
recording promptTokens/totalTokens under the same `model` key-value
group used by chat metrics. Recording happens at the embedWithUsage
chokepoint, so direct calls and vector-store/RAG paths are both covered
without each provider repeating it.

Embedding calls do not run inside a plan action (unlike chat), so the
resource-bound metric group injected via setMetricGroup is used rather
than a request-scoped group handed in by an action. Embeddings record
totalTokens in place of chat's completionTokens since there is no
completion.

Tests mirror BaseChatModelSetupTokenMetricsTest for both languages.
Java cannot build locally (Java 11 required, host is Java 8); Python
verified locally (10 passed, ruff clean). Java verification relies on CI.

Closes apache#858

Generated-by: Claude Code 2.1.220 (glm-5.2[1m])
@github-actions github-actions Bot added doc-not-needed Your PR changes do not impact docs fixVersion/0.4.0 priority/major Default priority of the PR or issue. labels Aug 25, 2026
Apply the exact reformats CI's spotless:check reported: collapse the
Preconditions.checkArgument message onto the same line as the condition,
and the short "" assertThrows onto one line. No behavior change.

Generated-by: Claude Code 2.1.220 (glm-5.2[1m])

@weiqingy weiqingy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for taking this on. A few questions inline.

}

/** Descriptor args with an optional model (omitted when null/blank so it stays unset). */
private static Map<String, String> descriptorArgs(String model) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This helper returns Map<String, String>, but ResourceDescriptor's constructor takes Map<String, Object>. Java generics are invariant, so the call at :106 does not compile.

That is what all 18 red CI checks are hitting. Every one of them stops at flink-agents-api testCompile, including the Elasticsearch job, which builds this module through -am:

BaseEmbeddingModelSetupTokenMetricsTest.java:[106,70] incompatible types:
java.util.Map<java.lang.String,java.lang.String> cannot be converted to
java.util.Map<java.lang.String,java.lang.Object>

Would changing this helper and its local HashMap to Map<String, Object> be enough? I tried just that change locally, and the file compiles, all 9 new tests pass, and spotless:check stays green. The two sibling call sites already work because Map.of(...) at :156 and Collections.emptyMap() at :115 take their type from the target on the spot, so only the explicitly typed helper trips.

One thing worth knowing, since you mentioned leaning on CI: a green Code Style Check does not tell you the Java side builds. Spotless formats test sources without compiling them, so it stays green straight through a compile error.

* dimension.
*
* <p>Unlike the chat path, embedding calls do not run inside a plan action that hands in a
* request-scoped metric group (vector-store, RAG, and direct calls reach this setup directly),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This line says vector-store and RAG calls reach this setup, but they do not reach embedWithUsage, which is where the recording happens. All four call sites use embed: BaseVectorStore.java:179 (RAG query), BaseVectorStore.java:337 (auto-embed on add and update), vector_store.py:290, and vector_store.py:349.

I grepped embedWithUsage and embed_with_usage on main and found no vector-store or RAG call site at all. That matches your own Javadoc on embed(String), which says usage is discarded there. Those are the paths #858 asks for, so today they would still record nothing.

Is extending to the embed paths in scope for this PR? Or would you rather land the direct-call case first and reword this sentence, and the matching claim in the PR body, to match what it covers?

One more thing on the line above: it says embedding calls do not run inside a plan action, but RAG does. ContextRetrievalAction is registered as context_retrieval_action at ContextRetrievalAction.java:44. And while you are in the PR body, the sentence about RowTypeInfo and "a schema that renders" looks like it came from a different change. Worth dropping?

public void recordTokenMetrics(String modelName, long promptTokens, long totalTokens) {
Preconditions.checkArgument(
modelName != null && !modelName.isBlank(), "Model name must not be null or blank.");
FlinkAgentsMetricGroup metricGroup = getMetricGroup();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reading the group bound to the resource here, and at embedding_model.py:186, is the pattern #859 reported and #861 moved chat off.

Here is what worries me. Resource.metricGroup is a single mutable field on an object that is cached and shared across the whole subtask. RunnerContextImpl.getResource:485-495 rewrites it to the current action's group on every fetch. Actions yield to other keys while they run. So another action can rebind that field in between a ctx.getResource(...) call and the later embedWithUsage.

Chat takes a different route: it captures the group up front and passes it in (ChatModelInvoker.java:121, used at :176). There is a test guarding exactly that, BaseChatModelSetupTokenMetricsTest.java:95-111, which asserts the bound group is not used.

The vector-store path looks worse than racy. A vector store resolves its model through ResourceContext (BaseVectorStore.java:96-101), and that path never sets metricGroup at all, so it stays null there.

Given RAG does run inside an action, what would make passing the group in the way chat does hard here? And whichever way it settles, could a test pin it? None of the new tests bind two groups, so nothing would fail today if the choice were reversed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For context, this is the same recording-boundary issue that led #870 to remove automatic metric recording and retain only usage transport. @joeyutong is working on the framework-level fix, so this PR may need to wait for that work and then adopt the resulting API.

return EmbeddingModelUtils.toSingleEmbeddingResult(result);
EmbeddingResult<float[]> embeddingResult =
EmbeddingModelUtils.toSingleEmbeddingResult(result);
recordTokenUsage(embeddingResult.getTokenUsage());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this records the same tokens twice, for a Java agent using a Python embedding model.

adapter.invoke(CALL_EMBED_WITH_USAGE, ...) at :139 calls into the Python BaseEmbeddingModelSetup.embed_with_usage, and this PR makes that method record too, at embedding_model.py:159. Then this line records again.

Both writes land on the same Java SimpleCounter. setMetricGroup here (:177-181) binds the Java field and forwards the same FlinkAgentsMetricGroup to the Python side, and FlinkMetricGroup and FlinkCounter just pass through to that Java object. Neither guard stops it, because both model fields read the same descriptor argument (EmbeddingCrossLanguageAgent.java:68 sets it).

The other direction stays at one write: JavaEmbeddingModelSetupImpl.embed_with_usage (java_embedding_model.py:174-182) calls the Java resource without chaining to super().

Nothing would catch it today either. I deleted both recordTokenUsage(...) lines and the whole api module still passed 385/385. PythonEmbeddingModelSetupTest builds the setup from a @Mock ResourceDescriptor (:50), so getArgument("model") comes back null and the recording returns early, and no metric group is ever bound.

So: which side should own the write for a cross-language resource? Dropping these two lines and letting the Python setup own it would match what JavaEmbeddingModelSetupImpl already does, unless there is a reason the Java wrapper needs its own. Either way, would a test with a real descriptor carrying model plus a bound group be worth adding, so these lines are covered?

Called from ``embed_with_usage`` so direct calls and vector-store/RAG paths
are both covered without each provider repeating the recording.
"""
if token_usage is None or not self.model:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

not self.model and Java's model == null || model.isBlank() (BaseEmbeddingModelSetup.java:131) do not agree when the model name is only spaces. I ran the PR's own code to check: with model=' ' Python records under a group literally named model= , while Java skips it. With model='' both skip.

AGENTS.md asks for the Java, Python and YAML APIs to stay semantically aligned.

There is no Python test on this guard right now. Removing or not self.model leaves all 10 tests passing, while Java has testEmbedWithUsageNullModelRecordsNothing covering the same thing.

Is the whitespace case worth folding in? Something like this, in case it helps:

if token_usage is None or not (self.model or "").strip():

* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions of

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: permissions of looks like it wants to be permissions and. RAT accepts the file either way, so nothing is broken. It is the only Java file in the repo with that wording though. Worth folding into the compile fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs fixVersion/0.4.0 priority/major Default priority of the PR or issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants