Skip to content

Conversation

@amychisholm03
Copy link
Contributor

This PR resolves #3441

@codecov
Copy link

codecov bot commented Oct 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.68%. Comparing base (3ee4d38) to head (f2bfb17).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3467      +/-   ##
==========================================
- Coverage   97.80%   97.68%   -0.12%     
==========================================
  Files         428      431       +3     
  Lines       56592    56773     +181     
  Branches        1        1              
==========================================
+ Hits        55351    55461     +110     
- Misses       1241     1312      +71     
Flag Coverage Δ
integration-tests-cjs-20.x 73.60% <49.56%> (-0.37%) ⬇️
integration-tests-cjs-22.x 73.64% <49.56%> (-0.37%) ⬇️
integration-tests-cjs-24.x 74.37% <49.56%> (-0.38%) ⬇️
integration-tests-esm-20.x 52.51% <49.56%> (-0.12%) ⬇️
integration-tests-esm-22.x 52.56% <49.56%> (-0.12%) ⬇️
integration-tests-esm-24.x 53.73% <49.56%> (-0.14%) ⬇️
unit-tests-20.x 88.64% <49.56%> (-0.07%) ⬇️
unit-tests-22.x 88.67% <49.56%> (-0.07%) ⬇️
unit-tests-24.x 88.68% <49.56%> (-0.07%) ⬇️
versioned-tests-20.x 80.99% <100.00%> (-0.24%) ⬇️
versioned-tests-22.x 81.00% <100.00%> (-0.24%) ⬇️
versioned-tests-24.x 80.93% <100.00%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@amychisholm03 amychisholm03 marked this pull request as ready for review October 27, 2025 19:12
@amychisholm03 amychisholm03 force-pushed the NR-3441/google-genai-subscriber branch 2 times, most recently from e5ddbe6 to 802f0a4 Compare October 30, 2025 21:36
Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

Nice work, I had a few comments around some things. I think we can also file a ticket to abstract LLM based instrumentation to have a base LLM class to remove this copy pasta around recording messages, errors, etc but for a different story.

@amychisholm03 amychisholm03 force-pushed the NR-3441/google-genai-subscriber branch 2 times, most recently from 7dc5606 to bb58196 Compare November 3, 2025 20:46
@amychisholm03
Copy link
Contributor Author

agent.config -> config will be addressed in #3487

Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

We may have to re-check the spec for ai monitoring. Right now when the subscriber is registered it checks if ai_monitoring.enabled is set to true. If not, it will not subscribe. Any subsequent checks will never be false. This may be broken in openai as well. You should use this.config to check config, not this.agent.config. Yes they are the same thing but we're trying to create consistency.

@amychisholm03 amychisholm03 force-pushed the NR-3441/google-genai-subscriber branch from 8318d26 to 3bef21d Compare December 1, 2025 15:46
Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

I'm going to have a lot of the same comments:

  • remove optional chaining for checking configuration
  • use this.enabled to check if ai monitoring is enabled
  • add a this.streamingEnabled and use that to check if streaming is enabled
  • We should only be checking if ai monitoring is enabled before creating LLM events, the segment should still be created

@amychisholm03 amychisholm03 force-pushed the NR-3441/google-genai-subscriber branch from de814a2 to 94bb4bf Compare December 8, 2025 17:43
Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

A few comments inline but also this instrumentation has created some uncovered paths that need tests:

  • streaming is enabled but ai monitoring is not enabled
  • streaming and ai monitoring are enabled but no active transaction
  • embeddings when ai monitoring is disabled
  • embeddings when ai monitoring is enabled but not in active transaction

@bizob2828 bizob2828 changed the title refactor: Update @google/genai instrumentation to subscribe to events emitted refactor: Updated @google/genai instrumentation to subscribe to events emitted Dec 9, 2025
Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

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

LGTM, good work on this one

@amychisholm03 amychisholm03 merged commit e8d9ba1 into newrelic:main Dec 9, 2025
44 of 45 checks passed
@github-project-automation github-project-automation bot moved this from Needs PR Review to Done: Issues recently completed in Node.js Engineering Board Dec 9, 2025
@amychisholm03 amychisholm03 deleted the NR-3441/google-genai-subscriber branch December 9, 2025 16:45
@github-actions github-actions bot mentioned this pull request Dec 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done: Issues recently completed

Development

Successfully merging this pull request may close these issues.

Update google genai instrumentation to rely on tracing channel instrumentation

2 participants