zxs1633079383 commented on PR #870: URL: https://github.com/apache/flink-agents/pull/870#issuecomment-4922094141
Thanks @joeyutong, I agree with the concern. `cf989b4` fixes the previous mutable setup-bound metric-group problem, but it still lets ordinary caller threads perform metric lookup/recording. For vector-store auto-embedding, that means `query(...)` / auto-embedding can still call into lazy subgroup/counter resolution, and the counter update itself may happen from a concurrent resource API path. So this should not be treated as fully solved by just passing the request-scoped metric group explicitly. Before I make another implementation change, I think the right next step is to settle the abstraction boundary. Two possible shapes I am considering: 1. Introduce an `EmbeddingTokenMetricRecorder` (Java) / equivalent lightweight recorder on the Python side. The recorder would be created or refreshed at the existing action-thread resource binding point, where the action/request metric group is known. Later embedding/vector-store paths would only call `recorder.record(usage)` and would not perform `getSubGroup(...)` / `getCounter(...)` lazy lookup from arbitrary caller threads. 2. Keep the public `recordTokenMetrics(...)` shape, but make it use pre-resolved counter handles captured at binding time rather than resolving counters on each recording call. Either way, I think the recorder should avoid updating a non-thread-safe `SimpleCounter` concurrently. If the record path can still be reached from concurrent resource calls, then the handle should either wrap a thread-safe counter implementation such as `ThreadSafeSimpleCounter`, or the design should move the actual counter update back to a single-threaded action/mailbox boundary. This also seems related to the broader direction discussed in #861: the cached `Resource` should stay a stable capability, while per-action metric state belongs to a request/use context or bound recorder. I will hold off on rebasing/churning the PR until this direction is clear, then update the patch and rerun the branch against the latest `origin/main`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
