Skip to content

Conversation

@koxudaxi
Copy link
Collaborator

@koxudaxi koxudaxi commented Jul 8, 2025

No description provided.

@koxudaxi koxudaxi marked this pull request as ready for review July 8, 2025 07:00
@koxudaxi koxudaxi requested review from brenkao and willbakst July 8, 2025 07:10
@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2025

Codecov Report

Attention: Patch coverage is 58.15167% with 1363 lines in your changes missing coverage. Please review.

Project coverage is 89.74%. Comparing base (7a7d84f) to head (0d14608).

Files with missing lines Patch % Lines
...t/src/instrumentors/openai-otel-instrumentation.ts 11.15% 677 Missing ⚠️
sdks/typescript/src/instrumentors/openai-hook.ts 17.73% 283 Missing ⚠️
sdks/typescript/src/register-otel.ts 0.00% 136 Missing ⚠️
sdks/typescript/src/esm-hooks.mjs 0.00% 103 Missing ⚠️
sdks/typescript/src/register-esm-loader.mjs 0.00% 50 Missing ⚠️
sdks/typescript/src/span.ts 90.98% 34 Missing ⚠️
sdks/typescript/src/register.ts 73.80% 33 Missing ⚠️
sdks/typescript/src/configure.ts 89.34% 18 Missing ⚠️
sdks/typescript/src/exporters/json-exporter.ts 92.69% 13 Missing ⚠️
sdks/typescript/src/wrap-openai.ts 96.03% 8 Missing and 1 partial ⚠️
... and 3 more
Additional details and impacted files
@@                    Coverage Diff                     @@
##           update-typescript-sdk     #601       +/-   ##
==========================================================
- Coverage                  99.92%   89.74%   -10.18%     
==========================================================
  Files                        190      211       +21     
  Lines                      10115    13372     +3257     
  Branches                       0      226      +226     
==========================================================
+ Hits                       10107    12001     +1894     
- Misses                         8     1370     +1362     
- Partials                       0        1        +1     
Flag Coverage Δ
App-ubuntu-latest-python3.10 99.87% <ø> (ø)
App-ubuntu-latest-python3.11 99.87% <ø> (ø)
App-ubuntu-latest-python3.12 99.87% <ø> (ø)
App-ubuntu-latest-python3.13 99.87% <ø> (ø)
SDK-ubuntu-latest-python3.10 100.00% <ø> (ø)
SDK-ubuntu-latest-python3.11 100.00% <ø> (ø)
SDK-ubuntu-latest-python3.12 100.00% <ø> (ø)
SDK-ubuntu-latest-python3.13 100.00% <ø> (ø)
TypeScript-SDK-ubuntu-latest-node18 58.15% <58.15%> (?)
TypeScript-SDK-ubuntu-latest-node20 58.15% <58.15%> (?)
TypeScript-SDK-ubuntu-latest-node22 58.15% <58.15%> (?)

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.

Comment on lines 1 to 61
# AutoLLM Import Order Constraint

## Important Limitation

When using the `autoLlm: true` option, there is a critical constraint regarding module import order:

### ❌ This will NOT work:
```typescript
import OpenAI from "openai"; // OpenAI imported BEFORE configure()
import { configure } from "lilypad";

await configure({
autoLlm: true // Hook cannot intercept already-loaded module
});

const client = new OpenAI(); // Will NOT be instrumented
```

### ✅ This WILL work:
```typescript
import { configure } from "lilypad";

// Configure FIRST
await configure({
autoLlm: true
});

// Import OpenAI AFTER configure()
const { default: OpenAI } = await import("openai");
const client = new OpenAI(); // Will be instrumented correctly
```

## Technical Explanation

The `autoLlm` feature uses Node.js `Module._load` hooks to intercept and wrap the OpenAI module at load time. This approach has an inherent limitation:

- **Module._load hooks only work when a module is loaded for the first time**
- If the module is already in the require cache, the hook will not be triggered
- This is a fundamental constraint of Node.js module system, not specific to our implementation

## Recommended Approaches

### 1. Use Dynamic Import (Most Flexible)
```typescript
await configure({ autoLlm: true });
const { default: OpenAI } = await import("openai");
```

### 2. Use --require Flag (Most Reliable)
```bash
node --require ./dist/register.js your-app.js
```
This ensures hooks are installed before any application code runs.

### 3. Manual Wrapping (Most Control)
```typescript
import OpenAI from "openai";
import { wrapOpenAI } from "lilypad";

const client = wrapOpenAI(new OpenAI());
```
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@brenkao @willbakst
Unlike Python, there appear to be limitations when applying patches (module hooks). If you know of a solution, please share it.
We should also decide and document which approach to recommend as a library before publishing the package. Related to this, I've discovered that things become even more complicated when parent spans exist, so I'll comment on that in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants