fix(zpages): use non-deprecated attribute.Value.String for formatting - #9453
fix(zpages): use non-deprecated attribute.Value.String for formatting#9453om7057 wants to merge 6 commits into
Conversation
attribute.Value.Emit is deprecated in favor of Value.String, which implements the OpenTelemetry AnyValue representation for non-OTLP protocols. Emit also produces invalid output for some values, e.g. a float64 slice containing NaN renders as "invalid: [NaN 1.5]" instead of the spec-compliant ["NaN",1.5].
The test filtered on a fixed latency bucket, but which bucket a near-instant span lands in is timing-dependent and differs between environments (observed passing locally, failing in CI). Use an error span instead, which isn't latency-bucketed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9453 +/- ##
=====================================
Coverage 84.5% 84.5%
=====================================
Files 203 203
Lines 16805 16805
=====================================
+ Hits 14204 14213 +9
+ Misses 2121 2114 -7
+ Partials 480 478 -2
🚀 New features to boost your workflow:
|
|
@dashpole a small reminder on this! |
ps-mir
left a comment
ps-mir
left a comment
There was a problem hiding this comment.
Code change looks ok, a comment on covering regression for other cases.
Emit and String also disagree for FLOAT64 with +/-Inf ("+Inf"/"-Inf"
vs the spec's "Infinity"/"-Infinity") and for BOOLSLICE (space- vs
comma-separated elements). Cover those alongside the original
FLOAT64SLICE/NaN case.
|
@ps-mir I've resolved the comment, please check the changes made. |
ps-mir
left a comment
ps-mir
left a comment
There was a problem hiding this comment.
I think wantNot can be dropped, it adds conditional path only for 1 specific test which is already covered by positive assertion.
| body := w.Body.String() | ||
| assert.Contains(t, body, tc.want) | ||
| assert.NotContains(t, body, "invalid:") | ||
| if tc.wantNot != "" { |
There was a problem hiding this comment.
The positive assertion already takes care of this. I think we can remove wantNot handling here, and from the type definition above as well.
There was a problem hiding this comment.
Sure, dropped wantNot and the field from the struct.
The positive want assertion already pins the exact output, so a separate wantNot check is redundant.
|
@ps-mir done!
|
…ttribute-formatting
3 participants
Summary
Closes #9008.
zpagesformats span attributes using the deprecatedattribute.Value.Emit.attribute.Value.Stringis the documented replacement and follows the OpenTelemetry AnyValue representation for non-OTLP protocols.Beyond the deprecation,
Emitcan produce outright invalid output. For example, afloat64slice containingNaNrenders asinvalid: [NaN 1.5]viaEmit, whileStringrenders the spec-compliant["NaN",1.5].Test plan
NaN-containing float64 slice attribute through the real/tracezHTTP handler and asserts the spec-compliant formatting, replacing the previousinvalid:outputgo build ./...,go vet ./...,go test ./... -racepass inzpagesgolangci-lint runpasses with 0 issues