Skip to content

Conversation

@amychisholm03
Copy link
Contributor

@amychisholm03 amychisholm03 commented Nov 5, 2025

This PR resolves #3442.

@amychisholm03 amychisholm03 changed the title Nr 3442/langchain subscriber refactor: Updated @langchain/core instrumentation to subscribe to events emitted Nov 5, 2025
@codecov
Copy link

codecov bot commented Nov 5, 2025

Codecov Report

❌ Patch coverage is 99.40191% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.52%. Comparing base (74777d5) to head (301c283).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
lib/subscribers/base.js 44.44% 5 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (74777d5) and HEAD (301c283). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (74777d5) HEAD (301c283)
unit-tests-24.x 1 0
unit-tests-20.x 1 0
unit-tests-22.x 1 0
integration-tests-cjs-24.x 1 0
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3493      +/-   ##
==========================================
- Coverage   97.81%   89.52%   -8.29%     
==========================================
  Files         434      435       +1     
  Lines       56941    57091     +150     
  Branches        1        1              
==========================================
- Hits        55698    51112    -4586     
- Misses       1243     5979    +4736     
Flag Coverage Δ
integration-tests-cjs-20.x 73.67% <50.35%> (+0.11%) ⬆️
integration-tests-cjs-22.x 73.71% <50.35%> (+0.11%) ⬆️
integration-tests-cjs-24.x ?
integration-tests-esm-20.x 52.73% <50.35%> (+0.19%) ⬆️
integration-tests-esm-22.x 52.78% <50.35%> (?)
unit-tests-20.x ?
unit-tests-22.x ?
unit-tests-24.x ?
versioned-tests-20.x 81.08% <99.40%> (?)
versioned-tests-22.x 81.08% <99.40%> (-0.17%) ⬇️
versioned-tests-24.x 81.02% <99.40%> (-0.17%) ⬇️

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 force-pushed the NR-3442/langchain-subscriber branch from 51a3401 to f44078a Compare November 5, 2025 22:58
@amychisholm03 amychisholm03 marked this pull request as ready for review November 5, 2025 23:32
@amychisholm03 amychisholm03 force-pushed the NR-3442/langchain-subscriber branch from 9f9d6b0 to 7c0304a Compare November 6, 2025 23:02
@amychisholm03 amychisholm03 force-pushed the NR-3442/langchain-subscriber branch 2 times, most recently from 427f4c0 to a050bbb Compare November 11, 2025 21: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.

Same as google gen ai. we need to look at the spec for enabling/disabling AI monitoring, not sure if we have this correct

@amychisholm03 amychisholm03 force-pushed the NR-3442/langchain-subscriber branch from a050bbb to 4f874a9 Compare December 1, 2025 15:48
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-3442/langchain-subscriber branch 3 times, most recently from a51660c to 9fbf2c0 Compare December 8, 2025 18:40
if (!this.streamingEnabled) {
logger.warn('`ai_monitoring.streaming.enabled` is set to `false`, stream will not be instrumented.')
agent.metrics.getOrCreateMetric(STREAMING_DISABLED).incrementCallCount()
agent.metrics
Copy link
Member

Choose a reason for hiding this comment

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

this is fine but all the other AIM instrumentations wraps this in an addLlmMeta, that can happen in #3487

@amychisholm03 amychisholm03 force-pushed the NR-3442/langchain-subscriber branch from cb5d298 to c620c39 Compare December 9, 2025 17:51
@bizob2828 bizob2828 self-assigned this Dec 11, 2025
@amychisholm03 amychisholm03 force-pushed the NR-3442/langchain-subscriber branch from d2a7fd7 to 964d537 Compare December 11, 2025 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs PR Review

Development

Successfully merging this pull request may close these issues.

Update langchain instrumentation to rely on tracing channel instrumentation

2 participants