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

freeze structs/arrays; disallow multidimensional arrays #950

Closed
wants to merge 5 commits into from
Closed

freeze structs/arrays; disallow multidimensional arrays #950

wants to merge 5 commits into from

Conversation

lizthegrey
Copy link
Member

@lizthegrey
Copy link
Member Author

Hm, open-telemetry/opentelemetry-proto#157 says that yes, OTLP does intend to support nested arrays (even though the spec itself is silent on whether nested arrays are allowed). Let me rethink my approach here...

@lizthegrey
Copy link
Member Author

open-telemetry/opentelemetry-specification#596 says at the moment we are not required to support nested arrays. Out of simplicity, until that PR is passed, I think we should not support nested arrays, then :)

@lizthegrey lizthegrey added the blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made label Jul 21, 2020
@MrAlias MrAlias added this to Needs Triage in GA Burndown Aug 27, 2020
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.

Looks like the related spec issue closed with no change, meaning the single dimensionality is the current expected behavior.

GA Burndown automation moved this from Needs Triage to Reviewer approved Aug 27, 2020
@MrAlias MrAlias added priority:p1 and removed blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made labels Aug 27, 2020
@MrAlias MrAlias linked an issue Aug 27, 2020 that may be closed by this pull request
@MrAlias
Copy link
Contributor

MrAlias commented Sep 1, 2020

@lizthegrey I think this can be merged if you can clean up the conflicts. I tried myself, but it looks like contributions from maintainers is turned off for this source.

@lizthegrey
Copy link
Member Author

Great, sorry about the delay, I'll get to it this week.

@MrAlias
Copy link
Contributor

MrAlias commented Oct 1, 2020

We talked about this PR today at the Go SIG meeting and we're wondering @lizthegrey if you need someone to take over this work?

Additionally, there were questions about the support of slices. Currently all the methods are named and work with arrays. This is a useful format to store the data in as it is very optimized for storage, but it is not the common type used. Instead, commonly slices are the type used and if we are returning an array (each a distinct type for each length of the array) users cannot handle these values in a generalized way. I.e. they cannot pass the return value to a function as the function needs to be defined to support all the types each array could be, instead they will need to convert the array back to a slice. It seems like we might provide a better API if we support this by default.

@lizthegrey
Copy link
Member Author

Yeah, go ahead and take it over. I'm almost done moving but not quite yet set up.

@lizthegrey lizthegrey closed this Oct 1, 2020
GA Burndown automation moved this from Reviewer approved to Done Oct 1, 2020
@MrAlias MrAlias added this to Done in OpenTelemetry Go RC Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

kv.Array() does not make copy of slice
2 participants