Skip to content
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

log: Implement Stringer for Value and KeyValue #4952

Closed
pellared opened this issue Feb 21, 2024 · 3 comments · Fixed by #5117
Closed

log: Implement Stringer for Value and KeyValue #4952

pellared opened this issue Feb 21, 2024 · 3 comments · Fixed by #5117
Assignees
Labels
area:logs Part of OpenTelemetry logs pkg:API Related to an API package
Milestone

Comments

@pellared
Copy link
Member

The failure messages from assertions like this one (I changed the test to fail) convinced me to implement Stringer.

        	Error Trace:	/home/rpajak/repos/opentelemetry-go/log/keyvalue_test.go:81
        	Error:      	Not equal: 
        	            	expected: false
        	            	actual  : true
        	Test:       	TestValueEqual
        	Messages:   	{[] 2 0xc0001a4410}.Equal({[] 2 0xc0001a4410})

Reference: #4809 (comment)

Originally posted by @pellared in #4949 (comment)

@pellared pellared added area:logs Part of OpenTelemetry logs pkg:API Related to an API package labels Feb 21, 2024
@pellared
Copy link
Member Author

pellared commented Feb 26, 2024

I would like to have consensus if we want to have it in initial stable release.

@MrAlias, do I remember correctly that you suggested removing this issue from the Logs RC project as it is NOT required by the OTel spec (it is only a convenience feature)? If so I am even leaning to closing this issue.

@MrAlias
Copy link
Contributor

MrAlias commented Feb 26, 2024

I'm not opposed to adding this. There are two issues though:

  • The attribute package has a lot more configuration for string output with its encoders. Are we going to hit an issue if we make a single choice for how KeyValue are printed here?
  • Will the comparable string be used as a comparison for equality checks or maps? Whether we want to or now users would then have a way to compare two Value outside of Equal. If we want this an encoder might be a better approach.

@pellared
Copy link
Member Author

The `attribute package has a lot more configuration for string output with its encoders. [...] If we want this an encoder might be a better approach.

Cannot the user create its own encoding (and even decoding) API that takes log.Value and log.KeyValue? What is the use case of allowing a user-defined Encoder? Is there a place in e.g. Go standard library that allows custom encoding? I think that we can provide our opinionated encoding like e.g. https://pkg.go.dev/time.

Whether we want to or now users would then have a way to compare two Value outside of Equal.

I think it would be a user error.

We could add a doc comment like (inspired by https://pkg.go.dev/time#Time.String):

The returned string is meant for debugging; the representation is not stable.

@dmathieu dmathieu changed the title log: Implement Strigner for Value and KeyValue log: Implement Stringer for Value and KeyValue Feb 28, 2024
@pellared pellared self-assigned this Mar 29, 2024
@MrAlias MrAlias added this to the v1.25.0 milestone Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logs Part of OpenTelemetry logs pkg:API Related to an API package
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants