Use the short error.type name in workflow observability to match other spans and Python#697
Conversation
3c66382 to
d1b750a
Compare
There was a problem hiding this comment.
Pull request overview
Aligns workflow observability span attributes with the rest of the repo (and Python SDK parity) by emitting short, unqualified error.type / build.error.type names via otelx.ErrorTypeName instead of reflect.TypeOf(err).String().
Changes:
- Replace
reflect.TypeOf(err).String()withotelx.ErrorTypeName(err)in workflowCaptureError,BuildErrorAttributes, andErrorAttributes. - Drop the
reflectimport and addinternal/otelxdependency for workflow observability. - Add unit tests covering short type names for
ErrorAttributes,BuildErrorAttributes, andCaptureErrorviaStartWorkflowRunusing a fake tracer/span.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| workflow/internal/observability/observability.go | Switch workflow error-type attributes to use otelx.ErrorTypeName for consistent short error.type values. |
| workflow/internal/observability/observability_test.go | Add tests asserting short error type names for workflow observability attributes and capture path. |
Comments suppressed due to low confidence (1)
workflow/internal/observability/observability_test.go:53
- This test asserts the exact stdlib-internal wrapped error type name ("wrapError"), which is not a stable API surface and may change between Go versions. Consider asserting the “short/unqualified” property instead to avoid version-coupled failures.
wrapped := fmt.Errorf("ctx: %w", errors.New("boom"))
attrs = observability.BuildErrorAttributes(wrapped)
if got := attributeValue(t, attrs, observability.TagBuildErrorType); got != "wrapError" {
t.Errorf("wrapped build.error.type = %q, want %q", got, "wrapError")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wrapped := fmt.Errorf("ctx: %w", errors.New("boom")) | ||
| attrs = observability.ErrorAttributes(wrapped) | ||
| if got := attributeValue(t, attrs, observability.TagErrorType); got != "wrapError" { | ||
| t.Errorf("wrapped error.type = %q, want %q", got, "wrapError") | ||
| } |
There was a problem hiding this comment.
Good catch — fixed in 23bbbaf: the wrapped-error cases now assert the observable requirement (a non-empty, unqualified/short type name with no package qualifier) instead of the stdlib-internal wrapError name, so the test won't break across Go releases.
| _, activity := telemetry.StartWorkflowRun(context.Background(), observability.WorkflowMetadata{ID: "wf"}) | ||
| activity.CaptureError(errors.New("boom")) |
There was a problem hiding this comment.
Good catch — fixed in 23bbbaf: switched to t.Context() for consistency with provider/otelprovider/otel_test.go.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The workflow observability spans set error.type/build.error.type via reflect.TypeOf(err).String(), yielding qualified names like *errors.errorString and *fmt.wrapError. Every other error.type site in the repo (otelprovider, tool auto-call) uses otelx.ErrorTypeName, which returns the short unqualified name (errorString, wrapError) to match Python's type(exception).__name__. Switch the three workflow sites to otelx.ErrorTypeName so the same attribute is consistent across provider, tool, and workflow spans and aligned with the Python SDK.
Assert the observable unqualified/short type-name requirement instead of a stdlib-internal type name (wrapError) that is not a public API, and propagate a cancellable t.Context() in the CaptureError test.
23bbbaf to
5b5fc16
Compare
Parity Review: ✅ ApprovedThis PR only modifies Cross-SDK parity: The change explicitly improves parity. It replaces
No Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
What
Workflow observability spans set the
error.typeandbuild.error.typeattributes usingreflect.TypeOf(err).String()at three sites inworkflow/internal/observability/observability.go(CaptureError,BuildErrorAttributes,ErrorAttributes). Forerrors.Newthat yields*errors.errorStringand forfmt.Errorf(...: %w)it yields*fmt.wrapError.This replaces those calls with
otelx.ErrorTypeName(err)(thereflectimport is dropped andinternal/otelxadded). Theerr == nilguards are unchanged.Why
Every other
error.typesite in the repo already usesotelx.ErrorTypeName— seeprovider/otelprovider/otel.goandagent/harness/toolautocall/autocall.go.ErrorTypeNameformats%Tand strips through the last dot, producing the short unqualified name (errorString,wrapError), and is documented to match Python'stype(exception).__name__.Before this change the same
error.typeattribute carriederrorStringon provider/tool spans but*errors.errorStringon workflow spans — an intra-repo inconsistency and a cross-SDK parity break with the Python SDK. This aligns the workflow path with the provider/tool paths and with Python.Tests
Added
workflow/internal/observability/observability_test.go:ErrorAttributes(errors.New("boom"))andBuildErrorAttributes(...)assert the type value equalserrorString, not*errors.errorString.fmt.Errorf("ctx: %w", ...)assertswrapError, not*fmt.wrapError.CaptureErroris driven throughStartWorkflowRunwith a fake tracer/span and asserts the capturederror.typeequalserrorString.go build ./...,go vet ./workflow/internal/observability/..., andgo test ./workflow/internal/observability/...all pass; the new tests fail before the fix and pass after.