Skip to content

Fix/hashvalue unknown type panic - #8723

Closed
sarthaks2378 wants to merge 2 commits into
open-telemetry:mainfrom
sarthaks2378:fix/hashvalue-unknown-type-panic
Closed

sarthaks2378 wants to merge 2 commits into
open-telemetry:mainfrom
sarthaks2378:fix/hashvalue-unknown-type-panic

Conversation

@sarthaks2378

@sarthaks2378 sarthaks2378 commented Aug 10, 2026 •

Copy link
Copy Markdown

Fixes #8047

Problem

hashValue's default case panics whenever a Value's Type() doesn't
match any of the known type constants:

https://github.com/open-telemetry/opentelemetry-go/blob/main/attribute/hash.go#L284-L289

This is unreachable through the public API today — every exported
constructor in this package (BoolValue, IntValue, StringValue, ...)
sets vtype to one of the handled constants, so no user-facing code path
can currently trigger it.

However, Value.String and Value.AsInterface already treat this exact
same "unreachable" case defensively, returning "unknown" and an empty
interface value respectively instead of panicking:

https://github.com/open-telemetry/opentelemetry-go/blob/main/attribute/value.go#L470-L473
https://github.com/open-telemetry/opentelemetry-go/blob/main/attribute/value.go#L428-L431

hashValue was the one remaining place (per #8047 and the discussion on
#8038) that still panics instead of degrading gracefully. Since hashValue
backs Set.Equivalent, Hasher.Distinct, and ultimately attribute-set
deduplication throughout the SDK, a panic here is more consequential than
in String/AsInterface: it would propagate out of NewSet/Hasher.Write
and crash the caller.

Fix

  • Add a new unknownID hash constant (following the existing pattern of
    boolID, emptyID, etc.) and hash that in the default case instead of
    building an error message and panicking.
  • Drop the now-unused fmt import from attribute/hash.go.
  • Add TestHashValueUnknownType, which constructs a Value with an
    out-of-range vtype (only possible via package-internal field access,
    since the field is unexported) and asserts:
    1. hashValue no longer panics for it.
    2. The resulting hash is non-zero and stable.
    3. The hash does not depend on any of Value's other fields (numeric,
      stringly), since none of them are meaningful once the type is
      unknown.

Why this is safe

  • Purely additive/defensive: no behavior changes for any of the existing,
    reachable Value types (BOOL, INT64, FLOAT64, STRING, the slice
    types, SLICE, MAP, EMPTY) — this PR only touches the default
    branch that is currently unreachable.
  • No performance impact: the changed code path is the fallback branch, not
    the hot path for any real type.
  • Backward compatible: callers who happened to rely on a panic here (none
    should, since it's unreachable) would instead get a valid, stable
    Distinct/Set hash.

Test plan

  • go test ./attribute/... — all existing tests pass.
  • go vet ./attribute/... and gofmt -l attribute/ — clean.
  • New TestHashValueUnknownType exercises the previously-panicking path
    directly.

hashValue's default case panicked when a Value's Type() didn't match
any of the known constants. This is unreachable through the public
API, since every exported constructor sets vtype to one of the known
constants, but Value.String and Value.AsInterface already handle this
same unreachable case defensively (returning "unknown" / an empty
interface value) instead of panicking.

Make hashValue consistent with those two: fall back to a new stable
unknownID hash instead of panicking, so that Distinct()/Set hashing
can never crash a caller.

Also drops the now-unused fmt import.
Adds TestHashValueUnknownType, which constructs a Value with an
out-of-range vtype (only possible via package-internal field access,
since the field is unexported) and asserts hashValue no longer panics
and instead returns a stable hash that doesn't depend on the other
Value fields.
@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 10, 2026 •

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.1%. Comparing base (982d73e) to head (25e083e).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #8723   +/-   ##
=====================================
  Coverage   84.0%   84.1%           
=====================================
  Files        329     329           
  Lines      26068   26066    -2     
=====================================
+ Hits       21917   21924    +7     
+ Misses      3769    3760    -9     
  Partials     382     382           
Files with missing lines Coverage Δ
attribute/hash.go 98.9% <100.0%> (+2.1%) ⬆️

... and 1 file with indirect coverage changes

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

@MrAlias

MrAlias commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Thanks for taking this on. We don't want to remove this panic. Value.vtype is private, so reaching this branch means we introduced an internal bug—most likely by adding a Type without adding its hashing implementation. The panic is intended to make that omission fail loudly during development and tests.

Hashing unknownID instead hides the omission and gives different values of the missed type the same Distinct, which can silently combine metric streams. The defensive fallbacks in String and AsInterface are not equivalent because they do not define attribute-set identity. This use is consistent with the Go guidance on panics as invariant checks, and it matches the original intent of #8047.

Since removing this invariant check is the purpose of the PR, I don’t think there is a change to request here. I’m going to close this PR. If other maintainers disagree we can re-evaluate.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

attribute: panic in places that should be unreachable

2 participants