Skip to content

Fix semconv generated error type to check error chain for custom type declaration#7994

Merged
pellared merged 8 commits intoopen-telemetry:mainfrom
MrAlias:semconv-err-type
Mar 4, 2026
Merged

Fix semconv generated error type to check error chain for custom type declaration#7994
pellared merged 8 commits intoopen-telemetry:mainfrom
MrAlias:semconv-err-type

Conversation

@MrAlias
Copy link
Copy Markdown
Contributor

@MrAlias MrAlias commented Mar 3, 2026

Fix ErrorType detection to work through wrapped error chains.

errorType previously only checked whether the top-level error value implemented ErrorType() string. In common Go usage, errors are often wrapped, so this could miss ErrorType implementations behind wrappers.

@MrAlias MrAlias added this to the v1.42.0 milestone Mar 3, 2026
@MrAlias MrAlias added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Mar 3, 2026
@MrAlias MrAlias marked this pull request as ready for review March 3, 2026 17:47
Copilot AI review requested due to automatic review settings March 3, 2026 17:47
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 3, 2026

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.6%. Comparing base (b2b3250) to head (d3b3479).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
semconv/v1.40.0/error_type.go 0.0% 4 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #7994     +/-   ##
=======================================
- Coverage   81.6%   81.6%   -0.1%     
=======================================
  Files        304     304             
  Lines      23385   23389      +4     
=======================================
- Hits       19091   19090      -1     
- Misses      3905    3911      +6     
+ Partials     389     388      -1     
Files with missing lines Coverage Δ
semconv/v1.40.0/error_type.go 0.0% <0.0%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

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 semconv ErrorType helper to detect custom ErrorType() string implementations through wrapped error chains (using errors.As), aligning behavior with common Go error-wrapping patterns.

Changes:

  • Update errorType to use errors.As so wrapped errors can contribute ErrorType() string.
  • Update doc comments to describe chain-aware ErrorType detection.
  • Extend generated tests (and templates) to cover wrapped errors.

Reviewed changes

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

File Description
semconv/v1.40.0/error_type.go Switches ErrorType detection from direct assertion to chain-aware errors.As; updates docs accordingly.
semconv/v1.40.0/error_type_test.go Adds coverage for wrapped errors and empty ErrorType values in the chain.
internal/tools/semconvkit/templates/error_type.go.tmpl Updates generator template to emit the new chain-aware implementation and docs.
internal/tools/semconvkit/templates/error_type_test.go.tmpl Updates generator template tests to validate wrapped-error behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Fixes #7975 ?

Shouldn't we unwrap even if it does not implement ErrorType() method?

@MrAlias
Copy link
Copy Markdown
Contributor Author

MrAlias commented Mar 3, 2026

Fixes #7975 ?

Shouldn't we unwrap even if it does not implement ErrorType() method?

I think that is a different discussion. I don't see it in the scope of this PR. This is addressing the linked feedback from the previous PR review addressing specifically the detection of the ErrorType() string interface.

@seh
Copy link
Copy Markdown
Contributor

seh commented Mar 4, 2026

I agree with @MrAlias that this patch does not fix #7975, even though it addresses a similar blindness with a similar remediation. The word isomorphic comes to mind.

@pellared pellared merged commit a7624f5 into open-telemetry:main Mar 4, 2026
30 of 31 checks passed
@MrAlias MrAlias deleted the semconv-err-type branch March 4, 2026 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants