Fix JSON marshaling of attribute.Set
#6780
Conversation
The committers listed above are authorized under a signed CLA. |
danielorbach
requested review from
MrAlias,
XSAM,
dashpole,
pellared and
dmathieu
as code owners
The Set type implemented json.Marshaler on a pointer receiver. Alas, an attribute.Set is sometimes used as a value in otel packages. Most notably, the NewSet function returns a value not a pointer. Implementing MarshalJSON on a value receiver, like MarshalLog, enables proper marshalling when a Set is used as a value that is not addressable.
With attribute.Set now implementing json.Marshaler on values instead of
pointer receivers, any structs that hold a nil pointer would print as
"null" instead of the previous "{}".
This commit updates failing tests that were detected by 'make test'.
This change was generated by 'make precommit'.
The new name looks and feels more like the rest of the package.
The extended test cases cover usages of a Set more completely.
danielorbach
force-pushed
the
bugfix/json-marshal-attributes-set
branch
from
0d3931a to
b968edf
Compare
|
Hello maintainers! I hope you find this small changeset pleasing. I've ecnountered this bug while running an example test that used stdout exporter. As a work around in my local work, I'm using a custom encoder to check that the attributes I've set when creating the meter ( P.S. Now that you see JSON output concretely in the tests I've added, let me know if you prefer I further modify the function to return prettier output. |
| @@ -382,7 +382,7 @@ func computeDistinctReflect(kvs []KeyValue) interface{} { | |||
| } | |||
|
|
|||
| // MarshalJSON returns the JSON encoding of the Set. | |||
| func (l *Set) MarshalJSON() ([]byte, error) { | |||
| func (l Set) MarshalJSON() ([]byte, error) { | |||
There was a problem hiding this comment.
This changes the signature of a stable module. It is not something we can accept.
There was a problem hiding this comment.
Oh 😔 I see.
Do you agree with the problem I'm trying to solve?
I think the behaviour is unexpected because the output is inconsistent:
- a non-empty Set marshals to JSON as an array of one or more elements (
[{...}]) - an empty Set marshals to JSON as an empty object (
{}) - a pointer to an empty Set marshals to JSON as an empty array (
[]) - a nil pointer to Set marshals to JSON as an empty object (
{})
Im here to help with your guidance 🙏🏻
There was a problem hiding this comment.
Not sure there is a great solution here. I would look at the places that use the value type as a field and see if adding a custom JSON marshal function there would work.
There was a problem hiding this comment.
Hmmm 🤔 I will do that and update on this pull-request.
As another alternative, what is your opinion on changing those fields to point to an attribute.Set?
There was a problem hiding this comment.
It depends on stability and performance implications in each case.
There was a problem hiding this comment.
@MrAlias I've been looking at it some more. It appears the repository contains mixed references to attribute.Set as both value and pointer. I can provide examples if you desire.
The way I understand it, the type has all pointer receivers, meaning it is meant to be used as a pointer1. Alas, aside from a few places, it is being used as a value2.
Ultimately, trying to use it as a pointer internally is feasible, but I'll have to evaluate it some more so as to not break more compatibility in the module's exposed API.
So, we are left with your original suggestion to implement some custom marshalling on the types that expose an attribute.Set as a field. Would this specialisation not be a breaking change, in the same way that modifying the receiver of Set.MarshalJSON is?
I guess what I'm trying to ask, why would you consider my original suggestion as a breaking change? Though technically changing the signature, it is an inclusive change in the sense that all pointers can be treated as values, but the opposite is not always true. Just like most stdlib types with pointer receivers still implement fmt.Stringer on a value receiver.
I mean, other than a few hard-coded strings comparing the output of a few peripheral structs, what would we actually be breaking? I've put time into reading more and more sections of this repository, and I've been using OpenTelemetry for three years now. These experiences lead me to believe that we would be chaning the textual output for some users (as evident in this repo also), though not breaking API compatiblity per-se.
My paradox here is that if breaking the consistency of the module's JSON marshalling output is frowned upon, then by definition any attempt to improve it is a violation regardless.
If you are uncomfortable with any other modification, my personal pain would be resolved by changing the output of just the stdout exporters, but I feel like that is small minded.
Sorry for using so many words 🤭 🤭 Point me in a direction that you see fit.
Footnotes
Currently, the
attribute.Settype implementjson.Marshaleron a pointer receiver. However, various otel packages use it as a value, not a pointer. Those values are often not addressable (i.e.reflect.Value.CanAddrreturnsfalse). Since anattribute.Setis astruct, thejsonpackage does not invoke its specialised implementation ofjson.Marshalerand falls back to reflection-based encoding of structs. Since this type has only unexported fields, the reflection encoding returns{}instead of the actual attributes.This pull-request changes the method's receiver to value kind. This change resembles the value receiver of
attribute.Set.MarshalLog. I've git blamed to see if either value or pointer received was intentional in those functions, yet it seems arbitrary, so I'm comfortable changing it.I've ran the tests in this repo, according to the contribution guidelines, and fixed tests that failed for expecting the litertal
{}fornilpointers to anattribute.Set.