openinference icon indicating copy to clipboard operation
openinference copied to clipboard

feat: google-adk instrumentation.

Open daavoo opened this issue 7 months ago • 4 comments

Closes #1506

From examples/tools.py:

  • LLM (with function call output)

Screenshot 2025-04-30 151426

  • TOOL

Screenshot 2025-04-30 151435

  • LLM (with text output)

Screenshot 2025-04-30 151445

daavoo avatar Apr 30 '25 13:04 daavoo

Thanks so much for contributing @daavoo ! If it's okay as I review I might push some fixes into your branch if it's low hanging fruit just to get a few more things over the line. I see some missing attributes like token-counts and just want to ship good first version for you!

mikeldking avatar May 01 '25 17:05 mikeldking

Thanks so much for contributing @daavoo ! If it's okay as I review I might push some fixes into your branch if it's low hanging fruit just to get a few more things over the line. I see some missing attributes like token-counts and just want to ship good first version for you!

No problemo, go ahead. I wanted to open the PR to check with you if the implementation (wrapping the callback properties) makes sense but I was not sure if I was filling the fields correctly (or if some important ones are missing)

daavoo avatar May 01 '25 18:05 daavoo

Thanks for this PR @daavoo. I think the current monkey-patching approach might run into problems when users create their own callbacks (as they're encouraged to do by the docs here). The main issue is that instance-level callbacks would override the instrumentor callbacks we added at the class level. For instance, if a user sets their callbacks to None during instantiation, the instrumentor would stop working. I'm looking into another solution that should fix these issues.

RogerHYang avatar May 07 '25 19:05 RogerHYang

Thanks for this PR @daavoo. I think the current monkey-patching approach might run into problems when users create their own callbacks (as they're encouraged to do by the docs here). The main issue is that instance-level callbacks would override the instrumentor callbacks we added at the class level. For instance, if a user sets their callbacks to None during instantiation, the instrumentor would stop working.

Yeah 😅 , I noticed that, I just put together something that worked in our context (any-agent) because we can choose to always call instrument manually after agent instantiation. But I agree is not optimal for general use cases🙏

I'm looking into another solution that should fix these issues.

In case it helps, I also looked into:

daavoo avatar May 07 '25 21:05 daavoo

Closing this, as commented the callback patch is not the right approach for openinference use case

daavoo avatar May 14 '25 09:05 daavoo

@daavoo if you don't mind, i think we can keep this PR open as we update the approach here given what we have so far

RogerHYang avatar May 14 '25 11:05 RogerHYang