Split pdata package without using aliases

[dmitryax]

This change demonstrates the approach outlined as Option 2 in #4832.

It splits /model/pdata package into multiple packages per signal using the following structure:

/model
  /plog
  /ptrace
  /pmetric

The naming is different from what is suggested in #4833 just to illustrate an example. IMO naming suggested in #4833 would work better with names of other packages and data structures within the collector repo.

This change introduces the following public functions that are needed only by the p<signal> packages from pcommon package:

• func InstrumentationLibraryFromOrig(orig *otlpcommon.InstrumentationLibrary) InstrumentationLibrary
• func AttributeValueFromOrig(orig *otlpcommon.AnyValue) AttributeValue
• func AttributeMapFromOrig(orig *[]otlpcommon.KeyValue) AttributeMap
• func ResourceFromOrig(orig *otlpresource.Resource) Resource
• func SpanIDFromOrig(orig data.SpanID) SpanID
• func SetSpanID(spanID SpanID, orig data.SpanID)
• func TraceIDFromOrig(orig data.TraceID) TraceID
• func SetTraceID(traceID TraceID, orig data.TraceID)

This change also introduces aliases from model/pdata to model/p<signal> for backward compatibility that can be removed once clients are migrated to model/p<signal>

[Aneurysm9]

I think that New<x>FromOTLP() would be an improvement over <x>FromOrig() as "Orig" doesn't provide much context and is only the name of the parameter. I think this would also clarify that, should our OTLP structs be made non-internal, or should we use the structs from otel-proto-go, these functions would be generally useful.

[Aneurysm9]

The naming is different from what is suggested in #4833 just to illustrate an example. IMO naming suggested in #4833 would work better with names of other packages and data structures within the collector repo.

Did you mean #4918? I don't see a naming proposal in #4833 that looks relevant here, but I agree that the structure proposed in #4918 would probably be preferable.

[dmitryax]

I think that NewFromOTLP() would be an improvement over FromOrig() as "Orig" doesn't provide much context and is only the name of the parameter. I think this would also clarify that, should our OTLP structs be made non-internal, or should we use the structs from otel-proto-go, these functions would be generally useful.

I agree we should have New prefix, but not sure about OTLP. It's not always the internal OTLP proto, in some cases it's other internal structures wrapping primitive types.

Did you mean #4918? I don't see a naming proposal in #4833 that looks relevant here, but I agree that the structure proposed in #4918 would probably be preferable.

You're right, I meant #4918

[Aneurysm9]

It's not always the internal OTLP proto, in some cases it's other internal structures wrapping primitive types.

Sure, but in those cases we don't need those exported initializers, do we? I don't think that the pdata.SpanID type currently has such an initializer. It just takes a byte array and calls the internal initializer for that type and all of its methods forward to the internal type. Is there a reason we don't just expose the internal type and eliminate the indirection?

[dmitryax]

Sure, but in those cases we don't need those exported initializers, do we? I don't think that the pdata.SpanID type currently has such an initializer.

SpanID/TraceID doesn't have an initializer but the struct is used directly https://github.com/open-telemetry/opentelemetry-collector/blob/main/model/pdata/generated_log.go#L604.

It just takes a byte array and calls the internal initializer for that type and all of its methods forward to the internal type. Is there a reason we don't just expose the internal type and eliminate the indirection?

I think even we remove the indirection, we still want to restrict access the the private [8]byte field. In that case we still need these NewTrace(Span)IDFromOrig functions because SpanID and TraceID are both used by traces and logs modules

[Aneurysm9]

I think even we remove the indirection, we still want to restrict access the the private [8]byte field. In that case we still need these NewTrace(Span)IDFromOrig functions because SpanID and TraceID are both used by traces and logs modules

Yes, but they wouldn't be exposing internal types. The whole reason for jumping through hoops with internal packages and non-public initializers is that we don't want to expose the OTLP implementation we're using. That's a non-issue for span and trace IDs as they're well-defined to be [8]byte and [16]byte. Even if we want to wrap that in a struct to prevent direct access to the bytes there is no reason to hide the initializer.