Panic in marshalling ResourceSpans

[pkositsyn]

Describe the bug
Marshalling otlpcollectortrace.ExportTraceServiceRequest panics if there are nil components in ResourceSpans (e.g. nil InstrumetationLibrarySpans). Can we fix that somehow?

[bogdandrutu]

Yes, can you provide a test that proves this?

[pkositsyn]

Created PR with test that panics

[tigrannajaryan]

@Vemmy124 thanks a lot for the repro. Since you went this far are you perhaps also able provide the fix for the bug?

[bogdandrutu]

@tigrannajaryan I would like to discuss the solution here, because I think it implies a lot of work. Already started to think about this.

[bogdandrutu]

One option that I have in mind and looking to do is actually to change all fields to be not-nullable, which will avoid this case.

[pkositsyn]

BTW, what was the purpose of nullable fields? I don't really understand. There are comments everywhere like "panics on non-initialized structure" and it's very hard to understand semantical difference between nil and zero-initialized structure

[bogdandrutu]

That is how protobuf by default generates repeated fields (slices), as a slice to pointer of objects. I think this is historical because iterating with range over a slice of objects (not pointers) was slow (it may still be, not sure if it was fixed) because for _, obj := range slice{} will copy the object.

Here is a proposal that includes your test: #2002

[bogdandrutu]

We modeled a bit too much on the proto generated code to avoid allocating nil objects. But I think we can make all the objects not nullable and remove that confusion

[pkositsyn]

@bogdandrutu @tigrannajaryan SGTM, would you like me to provide PR for that or we're still trying to agree on the idea?

[pkositsyn]

As long as there are no nil objects now, we can close the issue