Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update reporting functions to include a partial context in the error #28

Merged
merged 16 commits into from
Apr 9, 2025

Conversation

xnanodax
Copy link
Contributor

@xnanodax xnanodax commented Mar 26, 2025

  • add opts to change reporting behavior with create draft -> active (default behavior is warning)
    • add tests
type ErrorBehavior = 'error' | 'error-and-continue' | 'warn-and-continue'

export interface TransitionDraftOptions {
  previouslyActivatedBehavior?: ErrorBehavior
  invalidRelatedToBehavior?: ErrorBehavior
}
  • include partial context in error when reporting errors
  • consolidate error report functions

descoped:

  • add matchingIndex to start/end spans for computed spans

@xnanodax xnanodax changed the base branch from main to next March 26, 2025 00:07
Base automatically changed from next to main April 1, 2025 21:50
@bbrzoska bbrzoska requested a review from Copilot April 1, 2025 23:46
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the reporting functions to include a partial context in the error by introducing new types and adding an option to change reporting behavior when transitioning a draft trace to active. Key changes include:

  • Addition of new types (PartialPossibleTraceContext and ReportErrorFn) in types.ts.
  • Updates in Tracer.ts and Trace.ts to optionally pass an error-reporting context via opts.
  • Adjustments in tests to validate the new reporting behavior.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/v3/types.ts Introduced new types supporting partial error context
src/v3/Tracer.ts Updated error reporting logic to conditionally use error vs warning
src/v3/TraceManagerWithDraft.test.ts Updated tests to reflect new reporting behavior and context checks
src/v3/TraceManager.ts Adjusted type definitions for reportWarningFn
src/v3/Trace.ts Updated transitionDraftToActive to pass opts and report context
Comments suppressed due to low confidence (2)

src/v3/Tracer.ts:109

  • [nitpick] Consider addressing the underlying type issue instead of using @ts-expect-error. Updating type definitions could eliminate the need for suppressing errors.
// @ts-expect-error TS doesn't like this type for some reason

src/v3/Trace.ts:1074

  • [nitpick] Evaluate the root cause of the TypeScript complaint and update the type definitions accordingly to remove the need for @ts-expect-error.
// @ts-expect-error TS doesn't like this type for some reason

Copy link
Contributor

@bbrzoska bbrzoska left a comment

Choose a reason for hiding this comment

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

looks good, left some notes!

did we cover all uses of reportErrorFn?

@xnanodax
Copy link
Contributor Author

xnanodax commented Apr 2, 2025

did we cover all uses of reportErrorFn?

yup! should have 🙏 !

@xnanodax xnanodax changed the base branch from main to next April 9, 2025 17:25
@@ -929,8 +931,12 @@ export class Trace<
VariantsT
> {
if (!this.input.relatedTo) {
throw new Error(
"Tried to access trace's activeInput, but the trace was never provided a 'relatedTo' input value",
this.traceUtilities.reportErrorFn(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

small clean up: replacing throw error with this.traceUtilities.reportErrorFn

@@ -1250,7 +1313,11 @@ export class Trace<
this.applyDefinitionModifications(inputAndDefinitionModifications)

this.wasActivated = true
this.stateMachine.emit('onMakeActive', undefined)

if (isDraft) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only drafts can switch the state to active

new Error(
"Tried to access trace's activeInput, but the trace was never provided a 'relatedTo' input value",
),
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the best way we can address this time for the time being

@@ -97,29 +98,8 @@ export class Tracer<
}

interrupt = ({ error }: { error?: Error } = {}) => {
const trace:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

consolidated these into getCurrentTraceOrWarn

@@ -153,28 +133,8 @@ export class Tracer<
VariantsT
>,
): void => {
const trace:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

consolidated these into getCurrentTraceOrWarn

): void => {
const trace = this.getCurrentTraceOrWarn()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the flow is we check if we have a current trace, if not, we do a blanket warning (not customizable)

once a current trace is confirmed, and we transitionDraftToActive, then we can use the new optional draft options (previouslyActivatedBehavior and invalidRelatedToBehavior)

this.traceUtilities.reportWarningFn(
new Error(
`You are trying to initialize a trace that has already been initialized before (${trace.definition.name}).`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, originally had the wrong message

}

// verify that trace is the same definition as the Tracer's definition
if (trace.sourceDefinition !== this.definition) {
this.traceUtilities.reportWarningFn(
new Error(
`You are trying to initialize a trace that is not the same definition as the Tracer's definition is different.`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, originally had the wrong message

@xnanodax xnanodax merged commit 500c409 into next Apr 9, 2025
3 checks passed
@xnanodax xnanodax deleted the report-error-fns branch April 9, 2025 21:13
Copy link

github-actions bot commented Apr 9, 2025

🎉 This PR is included in version 2.4.0-next.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants