feat(eve): fix the remaining 0.30 event gaps and correlate traces - #506
Merged
Conversation
🦋 Changeset detectedLatest commit: 088db35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
4 Skipped Deployments
|
Contributor
📝 WalkthroughWalkthroughChangesEve integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant EveTurn
participant Evlog
participant AISDKSpan
EveTurn->>Evlog: emit reasoning, message, and result events
Evlog->>Evlog: accumulate turn metadata
EveTurn->>AISDKSpan: start model-call span
Evlog->>AISDKSpan: attach request and session IDs
Evlog->>EveTurn: write wide-event metadata
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/docs/content/5.use-cases/5.eve.md`:
- Line 211: Update the correlation claim near defineEvlogInstrumentation to
require telemetry configuration that includes runtimeContext, such as
telemetry.includeRuntimeContext, before stating that model-call spans and their
children carry evlog.request_id and evlog.session_id; otherwise narrow the
wording to spans explicitly configured to include it.
In `@examples/eve/package.json`:
- Line 34: Regenerate pnpm-lock.yaml to match the dependency declared by
examples/eve/package.json: add the examples/eve importer and the resolved
eve@0.30.8 package entry, preserving the existing lockfile structure and
integrity metadata.
In `@packages/evlog/README.md`:
- Line 595: Update the prose sentence describing defineEvlogHook() in the README
to replace “lifecycle” with “life cycle,” preserving the existing meaning and
all other wording.
In `@packages/evlog/src/eve/index.ts`:
- Around line 706-716: Update the comments surrounding the reasoning and
response logging in the response-processing code to use factual, plain wording:
state that only reasoning size metrics are logged, never reasoning content, and
directly describe when response length is captured and how response text is
handled. Preserve the existing logging behavior and modify comments only.
In `@packages/evlog/test/eve.test.ts`:
- Around line 69-71: Update the runTurn test fixture around result.completed to
derive the result type from HookEventMap and pass it directly without casting
options.result to never. Keep runTurn type-checked against result.completed so
incompatible Eve options are detected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: de3efcd7-f230-4a71-a38b-2187ef313cf4
⛔ Files ignored due to path filters (2)
packages/evlog/test/toolkit/__snapshots__/api-surface.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.changeset/eve-close-remaining-gaps.mdapps/docs/content/5.use-cases/5.eve.mdexamples/eve/agent/instrumentation.tsexamples/eve/agent/tools/issue_refund.tsexamples/eve/package.jsonpackages/evlog/README.mdpackages/evlog/src/eve/index.tspackages/evlog/test/eve.test.ts
Contributor
|
Thank you for following the naming conventions! 🙏 |
commit: |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/evlog/src/eve/index.ts (3)
1415-1419: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the private implementation JSDoc.
buildInstrumentationContextis private. This comment only repeats behavior expressed by the function and its return type. Keep JSDoc on the public interface and factory. (github.com)As per coding guidelines, use comments only for constraints not expressible in code; do not paraphrase or narrate implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evlog/src/eve/index.ts` around lines 1415 - 1419, Remove the private implementation JSDoc immediately above buildInstrumentationContext, while preserving JSDoc on the public interface and factory.Source: Coding guidelines
1158-1187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve zero-length response metadata.
An empty
message.completedvalue is valid. The handler recordsmessage.lengthas zero, and the flush condition then drops the response metadata. Track presence separately and add the regression test. (github.com)
packages/evlog/src/eve/index.ts#L1158-L1187: set a response-present flag for every non-null message.packages/evlog/src/eve/index.ts#L716-L723: flush when the flag is set, not only whenresponseCharsis positive.packages/evlog/test/eve.test.ts#L676-L731: assert{ responseChars: 0, response: '' }in full message mode.As per coding guidelines, bug fixes require a failing regression test before the fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evlog/src/eve/index.ts` around lines 1158 - 1187, Preserve metadata for empty responses by first adding a failing regression assertion in packages/evlog/test/eve.test.ts lines 676-731 for full message mode expecting responseChars: 0 and response: ''. In packages/evlog/src/eve/index.ts lines 1158-1187, have the message.completed handler mark response presence for every non-null message, including empty strings; then update the flush condition at lines 716-723 to use that presence flag instead of requiring responseChars to be positive.Source: Coding guidelines
170-174: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPreserve the
result.completedtype contract.
TurnAccumulator.resultis written fromevent.data.resultinresult.completed, but it is declared asresult?: unknown; the Eve fixture already types this asHookEventMap['result.completed']['data']['result']. Use that Eve result type or a local alias soeve.resultkeeps the structured result shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/evlog/src/eve/index.ts` around lines 170 - 174, Update the TurnAccumulator result field to use the structured result type from HookEventMap['result.completed']['data']['result'] (or a local alias), instead of unknown, while preserving its optionality and the existing result.completed assignment flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/evlog/src/eve/index.ts`:
- Around line 1415-1419: Remove the private implementation JSDoc immediately
above buildInstrumentationContext, while preserving JSDoc on the public
interface and factory.
- Around line 1158-1187: Preserve metadata for empty responses by first adding a
failing regression assertion in packages/evlog/test/eve.test.ts lines 676-731
for full message mode expecting responseChars: 0 and response: ''. In
packages/evlog/src/eve/index.ts lines 1158-1187, have the message.completed
handler mark response presence for every non-null message, including empty
strings; then update the flush condition at lines 716-723 to use that presence
flag instead of requiring responseChars to be positive.
- Around line 170-174: Update the TurnAccumulator result field to use the
structured result type from HookEventMap['result.completed']['data']['result']
(or a local alias), instead of unknown, while preserving its optionality and the
existing result.completed assignment flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a6539fb0-5625-4b37-934e-823a1c991681
📒 Files selected for processing (2)
packages/evlog/src/eve/index.tspackages/evlog/test/eve.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
📚 Description
📝 Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation