-
Notifications
You must be signed in to change notification settings - Fork 1.2k
attribute: add String method to Value and KeyValue #7812
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.
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
base: main
Are you sure you want to change the base?
Conversation
This gives nicer results when you pass one to fmt.Println, for example. Signed-off-by: Bryan Boreham <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7812 +/- ##
=======================================
- Coverage 86.1% 86.0% -0.1%
=======================================
Files 302 302
Lines 22046 22050 +4
=======================================
- Hits 18991 18977 -14
+ Misses 2675 2673 -2
- Partials 380 400 +20
🚀 New features to boost your workflow:
|
| { | ||
| name: "string with spaces", | ||
| kv: attribute.String("key", "hello world"), | ||
| want: "{key:hello world}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it surprising that strings are not quoted, especially with embedded spaces like here.
But this is what Value.Emit() did, so leaving this here for comment.
| { | ||
| name: "invalid/uninitialized KeyValue", | ||
| kv: attribute.KeyValue{}, | ||
| want: "{:unknown}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should print something clearer?
Originally I wanted this for debugging purposes, so exactly what is in the object may be important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose <invalid>.
| { | ||
| name: "bool slice", | ||
| value: attribute.BoolSliceValue([]bool{true, false, true}), | ||
| want: "[true false true]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it was surprising to me that this one has no commas, while the int, float and string slices do.
|
I dispute the findings of the code coverage CI check. I added two functions which are completely covered by tests; how could this make coverage go down? |
| // String implements the Stringer interface, used when you pass an object to fmt.Println, etc. | ||
| func (kv KeyValue) String() string { | ||
| return "{" + string(kv.Key) + ":" + kv.Value.String() + "}" | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // String implements the Stringer interface, used when you pass an object to fmt.Println, etc. | |
| func (kv KeyValue) String() string { | |
| return "{" + string(kv.Key) + ":" + kv.Value.String() + "}" | |
| } | |
| // String returns key-value pair as a string, formatted like "key:value". | |
| // | |
| // The returned string is meant for debugging; | |
| // the string representation is not stable. | |
| func (kv KeyValue) String() string { | |
| return string(kv.Key) + ":" + kv.Value.String() | |
| } |
Why? To follow prior art https://pkg.go.dev/go.opentelemetry.io/otel/log#KeyValue.String (more: #5117).
| - Add `go.opentelemetry.io/otel/semconv/v1.39.0` package. | ||
| The package contains semantic conventions from the `v1.39.0` version of the OpenTelemetry Semantic Conventions. | ||
| See the [migration documentation](./semconv/v1.39.0/MIGRATION.md) for information on how to upgrade from `go.opentelemetry.io/otel/semconv/v1.38.0.`(#7783) | ||
| - Add `String` method to `attribute.Value` and `KeyValue`. (#7812) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Add `String` method to `attribute.Value` and `KeyValue`. (#7812) | |
| - Add `String` method to `attribute.Value` and `KeyValue` in `go.opentelemetry.io/otel/attribute`. (#7812) |
| // String implements the Stringer interface, used when you pass an object to fmt.Println, etc. | ||
| // Do not confuse with AsString, which you should call if you know the value is of String type. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // String implements the Stringer interface, used when you pass an object to fmt.Println, etc. | |
| // Do not confuse with AsString, which you should call if you know the value is of String type. | |
| // String returns Value's value as a string, formatted like [fmt.Sprint]. | |
| // | |
| // The returned string is meant for debugging; | |
| // the string representation is not stable. |
| { | ||
| name: "invalid/uninitialized KeyValue", | ||
| kv: attribute.KeyValue{}, | ||
| want: "{:unknown}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose <invalid>.
| // String implements the Stringer interface, used when you pass an object to fmt.Println, etc. | ||
| // Do not confuse with AsString, which you should call if you know the value is of String type. | ||
| func (v Value) String() string { | ||
| return v.Emit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we deprecate Emit so that users migrate to the new method?
This gives nicer results when you pass one to
fmt.Println, for example.Disclosure: Claude wrote the tests for me. I deleted about a third of the cases it came up with as uninteresting.
Fixes: #7810