Skip to content

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

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

Merged
merged 16 commits into from
Apr 9, 2025
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/stories/mockComponentsv3/traceManager.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ export const traceManager = new TraceManager({
},
// eslint-disable-next-line no-magic-numbers
generateId: () => Math.random().toString(36).slice(2),
reportErrorFn: (error) => {
reportErrorFn: (error, currentTraceContext) => {
// eslint-disable-next-line no-console
console.error(error)
console.error(error, currentTraceContext)
},
})

Expand Down
77 changes: 72 additions & 5 deletions src/v3/Trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,14 @@ import type {
CompleteTraceDefinition,
DraftTraceContext,
RelationSchemasBase,
ReportErrorFn,
TraceContext,
TraceDefinitionModifications,
TraceInterruptionReason,
TraceInterruptionReasonForInvalidTraces,
TraceManagerUtilities,
TraceModifications,
TransitionDraftOptions,
} from './types'
import { INVALID_TRACE_INTERRUPTION_REASONS } from './types'
import type {
Expand Down Expand Up @@ -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

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

this as Trace<any, RelationSchemasT, any>,
)
}
return this.input as ActiveTraceConfig<
Expand Down Expand Up @@ -1071,6 +1077,8 @@ export class Trace<
input.variant
}. Must be one of: ${Object.keys(definition.variants).join(', ')}`,
),
// eslint-disable-next-line @typescript-eslint/no-explicit-any
this as Trace<any, RelationSchemasT, any>,
)
}

Expand Down Expand Up @@ -1221,21 +1229,76 @@ export class Trace<
RelationSchemasT,
VariantsT
>,
) {
{
previouslyActivatedBehavior = 'warn-and-continue',
invalidRelatedToBehavior = 'warn-and-continue',
}: TransitionDraftOptions = {},
): void {
const { isDraft } = this
let reportPreviouslyActivated: ReportErrorFn<RelationSchemasT>
let overwriteDraft = true
switch (previouslyActivatedBehavior) {
case 'error':
reportPreviouslyActivated = this.traceUtilities.reportErrorFn
overwriteDraft = false
break
case 'error-and-continue':
reportPreviouslyActivated = this.traceUtilities.reportErrorFn
break
default:
reportPreviouslyActivated = this.traceUtilities.reportWarningFn
break
}

// this is an already initialized active trace, do nothing:
if (!isDraft) {
reportPreviouslyActivated(
new Error(
`You are trying to activate a trace that has already been activated before (${this.definition.name}).`,
),
// eslint-disable-next-line @typescript-eslint/no-explicit-any
this as Trace<any, RelationSchemasT, any>,
)
if (!overwriteDraft) {
return
}
}

let reportValidationError: ReportErrorFn<RelationSchemasT>
let useInvalidRelatedTo = true
switch (invalidRelatedToBehavior) {
case 'error':
reportValidationError = this.traceUtilities.reportErrorFn
useInvalidRelatedTo = false
break
case 'error-and-continue':
reportValidationError = this.traceUtilities.reportErrorFn
break
default:
reportValidationError = this.traceUtilities.reportWarningFn
break
}

const { attributes } = this.input

const { relatedTo, errors } = validateAndCoerceRelatedToAgainstSchema(
inputAndDefinitionModifications.relatedTo,
this.definition.relationSchema,
)

if (errors.length > 0) {
this.traceUtilities.reportWarningFn(
reportValidationError(
new Error(
`Invalid relatedTo value: ${JSON.stringify(
inputAndDefinitionModifications.relatedTo,
)}. ${errors.join(', ')}`,
),
// eslint-disable-next-line @typescript-eslint/no-explicit-any
this as Trace<any, RelationSchemasT, any>,
)
if (!useInvalidRelatedTo) {
return
}
}

this.activeInput = {
Expand All @@ -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

// we might already be active in which case we would have issued a warning earlier in this method
this.stateMachine.emit('onMakeActive', undefined)
}
}

/**
Expand Down
28 changes: 20 additions & 8 deletions src/v3/TraceManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ import type {
AllPossibleTraceContexts,
CompleteTraceDefinition,
ComputedValueDefinitionInput,
DraftTraceContext,
RelationSchemasBase,
ReportErrorFn,
SpanDeduplicationStrategy,
TraceDefinition,
TraceManagerConfig,
Expand Down Expand Up @@ -40,7 +42,7 @@ export class TraceManager<
configInput: Omit<
TraceManagerConfig<RelationSchemasT>,
'reportWarningFn'
> & { reportWarningFn?: (warning: Error) => void },
> & { reportWarningFn?: ReportErrorFn<RelationSchemasT> },
) {
this.utilities = {
// by default noop for warnings
Expand Down Expand Up @@ -95,12 +97,6 @@ export class TraceManager<
VariantsT
>(traceDefinition.requiredSpans)

if (!requiredSpans) {
throw new Error(
'requiredSpans must be defined, as a trace will never end otherwise',
)
}

const labelMatching = traceDefinition.labelMatching
? convertLabelMatchersToFns(traceDefinition.labelMatching)
: undefined
Expand Down Expand Up @@ -180,7 +176,11 @@ export class TraceManager<
VariantsT
> = {
...traceDefinition,
requiredSpans,
requiredSpans:
requiredSpans ??
[
// lack of requiredSpan is invalid, but we warn about it below
],
debounceOnSpans,
interruptOnSpans,
suppressErrorStatusPropagationOnSpans,
Expand All @@ -191,6 +191,18 @@ export class TraceManager<
this.utilities.relationSchemas[traceDefinition.relationSchemaName],
}

if (!requiredSpans) {
this.utilities.reportErrorFn(
new Error(
'requiredSpans must be defined along with the trace, as a trace will never end otherwise',
),
{ definition: completeTraceDefinition } as Partial<
// eslint-disable-next-line @typescript-eslint/no-explicit-any
DraftTraceContext<any, RelationSchemasT, any>
>,
)
}

return new Tracer(completeTraceDefinition, this.utilities)
}

Expand Down
Loading