Skip to content

Fix JSON marshaling of attribute.Set #6780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix JSON marshaling of attribute.Set #6780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8001e1c
attribute: fix JSON marshaling of Set
danielorbach May 15, 2025
bf90237
stdout/stdoutlog: fix expected attribute.Set literals
danielorbach May 15, 2025
5d2dcc1
attribute: assert JSON equality properly
danielorbach May 15, 2025
b918ecd
attribute: rename marshalling test to resemble other tests
danielorbach May 15, 2025
b968edf
attribute: extend scenarios for testing marshalling of Set
danielorbach May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion attribute/set.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes the signature of a stable module. It is not something we can accept.

Copy link
Author

@danielorbach danielorbach May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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 🙏🏻

@MrAlias

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends on stability and performance implications in each case.

Copy link
Author

Choose a reason for hiding this comment

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

@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

  1. like strings.Builder and bytes.Buffer, which are always returned and passed as a pointer. ↩

  2. Most notable metric.WithAttributeSet and the attribute.New* functions. ↩

return json.Marshal(l.equivalent.iface)
}

Expand Down
60 changes: 60 additions & 0 deletions attribute/set_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package attribute_test

import (
"encoding/json"
"reflect"
"regexp"
"testing"
Expand Down Expand Up @@ -348,6 +349,65 @@ func args(m reflect.Method) []reflect.Value {
return out
}

func TestJSONMarshaling(t *testing.T) {
valueTests := []struct {
name string
set attribute.Set
want string
}{
{
name: "Empty",
set: attribute.NewSet(),
want: "[]",
},
{
name: "NonEmpty",
set: attribute.NewSet(attribute.Int("A", 1)),
want: `[{"Key":"A","Value":{"Type":"INT64","Value":1}}]`,
},
}
for _, tt := range valueTests {
t.Run("Value/"+tt.name, func(t *testing.T) {
data, err := json.Marshal(tt.set)
require.NoError(t, err)
require.JSONEq(t, tt.want, string(data))
})
}

pointerTests := []struct {
name string
set *attribute.Set
want string
}{
{
name: "Nil",
set: nil,
want: "null",
},
{
name: "Empty",
set: pointerToSet(attribute.NewSet()),
want: `[]`,
},
{
name: "NonEmpty",
set: pointerToSet(attribute.NewSet(attribute.Int("A", 1))),
want: `[{"Key":"A","Value":{"Type":"INT64","Value":1}}]`,
},
}
for _, tt := range pointerTests {
t.Run("Pointer/"+tt.name, func(t *testing.T) {
data, err := json.Marshal(tt.set)
require.NoError(t, err)
require.JSONEq(t, tt.want, string(data))
})
}
}

func pointerToSet(set attribute.Set) *attribute.Set {
return &set
}

func BenchmarkFiltering(b *testing.B) {
var kvs [26]attribute.KeyValue
buf := [1]byte{'A' - 1}
Expand Down
4 changes: 2 additions & 2 deletions exporters/stdout/stdoutlog/exporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ func getJSON(now *time.Time) string {
timestamps = "\"Timestamp\":" + string(serializedNow) + ",\"ObservedTimestamp\":" + string(serializedNow) + ","
}

return "{" + timestamps + "\"EventName\":\"testing.event\",\"Severity\":9,\"SeverityText\":\"INFO\",\"Body\":{\"Type\":\"String\",\"Value\":\"test\"},\"Attributes\":[{\"Key\":\"key\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key2\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key3\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key4\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key5\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"bool\",\"Value\":{\"Type\":\"Bool\",\"Value\":true}}],\"TraceID\":\"0102030405060708090a0b0c0d0e0f10\",\"SpanID\":\"0102030405060708\",\"TraceFlags\":\"01\",\"Resource\":[{\"Key\":\"foo\",\"Value\":{\"Type\":\"STRING\",\"Value\":\"bar\"}}],\"Scope\":{\"Name\":\"name\",\"Version\":\"version\",\"SchemaURL\":\"https://example.com/custom-schema\",\"Attributes\":{}},\"DroppedAttributes\":10}\n"
return "{" + timestamps + "\"EventName\":\"testing.event\",\"Severity\":9,\"SeverityText\":\"INFO\",\"Body\":{\"Type\":\"String\",\"Value\":\"test\"},\"Attributes\":[{\"Key\":\"key\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key2\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key3\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key4\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"key5\",\"Value\":{\"Type\":\"String\",\"Value\":\"value\"}},{\"Key\":\"bool\",\"Value\":{\"Type\":\"Bool\",\"Value\":true}}],\"TraceID\":\"0102030405060708090a0b0c0d0e0f10\",\"SpanID\":\"0102030405060708\",\"TraceFlags\":\"01\",\"Resource\":[{\"Key\":\"foo\",\"Value\":{\"Type\":\"STRING\",\"Value\":\"bar\"}}],\"Scope\":{\"Name\":\"name\",\"Version\":\"version\",\"SchemaURL\":\"https://example.com/custom-schema\",\"Attributes\":null},\"DroppedAttributes\":10}\n"
}

func getJSONs(now *time.Time) string {
Expand Down Expand Up @@ -269,7 +269,7 @@ func getPrettyJSON(now *time.Time) string {
"Name": "name",
"Version": "version",
"SchemaURL": "https://example.com/custom-schema",
"Attributes": {}
"Attributes": null
},
"DroppedAttributes": 10
}
Expand Down