Skip to content

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

Merged
pellared merged 8 commits into
open-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 into
open-telemetry:mainfrom
MrAlias:semconv-err-type

Conversation

@MrAlias

@MrAlias MrAlias commented Mar 3, 2026

Copy link
Copy Markdown
Contributor

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

codecov Bot commented Mar 3, 2026

Copy link
Copy Markdown

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread semconv/v1.40.0/error_type.go Outdated
Comment thread semconv/v1.40.0/error_type.go Outdated
Comment thread internal/tools/semconvkit/templates/error_type.go.tmpl Outdated
Comment thread internal/tools/semconvkit/templates/error_type.go.tmpl Outdated

@pellared pellared left a comment

Copy link
Copy Markdown
Member

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

MrAlias commented Mar 3, 2026

Copy link
Copy Markdown
Contributor Author

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

seh commented Mar 4, 2026

Copy link
Copy Markdown
Contributor

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

5 participants