Skip to content

Conversation

@dashpole
Copy link
Contributor

Forked from #7175 (comment)

This adds a test for JSON marshaling of attribute sets.

@codecov
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.9%. Comparing base (9798759) to head (e6c5f5c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #7268   +/-   ##
=====================================
  Coverage   82.9%   82.9%           
=====================================
  Files        265     265           
  Lines      24894   24894           
=====================================
+ Hits       20639   20646    +7     
+ Misses      3879    3873    -6     
+ Partials     376     375    -1     

see 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.

@dashpole dashpole added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 28, 2025
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Thanks for the added tests, this is a good addition to the package.

The original comment inspiring this was about the unmarshlilng of this data. Do we need added checks for unmarshling, or is that just not a possible operation?

@dashpole
Copy link
Contributor Author

dashpole commented Aug 29, 2025

Currently, unmarshaling our attribute.Value type doesn't work properly. When I tried, it correctly unmarshaled the list and the key of each element, but the value was always invalid. I think we could implement UnmarshalJSON for the Value type to make that round-trip work properly, but it doesn't today. I figured for the purposes of unblocking #7175 it would be enough to test the MarshalJSON output.

See

func (v Value) MarshalJSON() ([]byte, error) {
var jsonVal struct {
Type string
Value any
}
jsonVal.Type = v.Type().String()
jsonVal.Value = v.AsInterface()
return json.Marshal(jsonVal)
}
. We have some custom logic for marshaling, but nothing for unmarshaling.

@MrAlias
Copy link
Contributor

MrAlias commented Aug 29, 2025

So if we add another hash field to the Set will the unmarshal behavior remain the same (produce invalid values), or will it start to error?

@dashpole
Copy link
Contributor Author

Ah, you can't unmarshal into a Set today, but you can unmarshal into a []KeyValue. The former still won't work, and the latter will still work since we aren't changing KeyValue at all.

@MrAlias MrAlias added this to the v1.38.0 milestone Aug 29, 2025
@dashpole dashpole merged commit 18424a4 into open-telemetry:main Aug 29, 2025
38 checks passed
@dashpole dashpole deleted the json_marshal_test branch August 29, 2025 16:03
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.

4 participants