joeyutong commented on PR #870: URL: https://github.com/apache/flink-agents/pull/870#issuecomment-4921390039
@zxs1633079383 Thanks for the update. Explicitly passing the request-scoped metric group fixes the mutable setup-bound metric group issue, but I think the thread-safety concern still remains. The current path still records metrics from the caller thread. For example, vector-store auto-embedding records inside `query(...)` / auto-embedding, which are ordinary resource APIs and may be called from `durableExecuteAsync` or other concurrent paths. The metric group assumes single-threaded access: subgroup/counter lookup is backed by lazy maps, and Flink's default `MetricGroup.counter(name)` uses `SimpleCounter`, which is explicitly not thread-safe. Without changing the broader resource abstraction, one possible direction is to pre-resolve the embedding metric handles at the existing action-thread binding point, e.g. from `RunnerContextImpl.getResource(...)` when it calls `Resource.setMetricGroup(...)` / Python `set_metric_group(...)`. `BaseEmbeddingModelSetup` could resolve the model subgroup and the `promptTokens` / `totalTokens` counters there, and later `recordTokenMetrics(...)` / vector-store auto-embedding would use only this pre-resolved recorder instead of calling `getSubGroup(...)` / `getCounter(...)` again. The recorder should also avoid updating `SimpleCounter` concurrently, or use a thread-safe counter implementation such as `ThreadSafeSimpleCounter`. Do you have any thoughts on where this should fit in the current abstraction? -- 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]
