joeyutong commented on PR #861: URL: https://github.com/apache/flink-agents/pull/861#issuecomment-4902263385
> Thanks for taking this on @jlon. > > Since `Resource` instances are shared across multiple Actions, reporting metrics using the `metricGroup` bound at creation time can lead to incorrect metric matching. This PR addresses the issue by passing the current Action’s `metricGroup` at reporting time, ensuring metrics are reported to the correct `Action` scope. > > In this case, I think `Resource` no longer need to hold a `metricGroup` field. I agree. This PR fixes the concrete bug locally for chat token metrics by passing the request/action metric group at reporting time, instead of reading the mutable metric group currently bound to the cached resource. But the underlying model issue is broader than this one recording path. A cleaner abstraction would be to separate the stable resource instance from the per-action resource-use context: - `Resource` is a stable cached capability. - `ResourceUseContext` carries action-scoped state, including the current action metric group. - Metric reporting uses the current `ResourceUseContext`, not mutable state stored on the resource. - Cross-language bridges propagate this context when invoking resource methods or resolving nested resources. One possible shape is for `ctx.getResource(...)` to return a bound resource handle internally, i.e. the cached `Resource` plus the current `ResourceUseContext`, even if the public API is migrated gradually. Under that model, `Resource` would no longer need a mutable `metricGroup` field. It would also simplify the cross-language propagation work in [#860](https://github.com/apache/flink-agents/pull/860): instead of making every Python wrapper participate in `setMetricGroup(...)`, the bridge can pass the current use context through the existing `callMethod` / `getResource` path. CC @weiqingy This is probably a larger refactor than this PR should take on, but I agree with the direction: action-scoped metric state should not be owned by the cached `Resource` instance. -- 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]
