Switch the otlp profiles root element to be ProfilesData

[dmathieu]

The OTLP profiles endpoint takes a ResourceProfiles as its root slice. However, the data model wants a ProfilesData.
See https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/profiles/v1development/profiles.proto#L46-L66

This was unseen until now because ProfilesData only held ResourceProfiles.
But now that we have the various index maps in there, we need to handle this properly.

[florianl]

Thanks for the quick fix!

[tigrannajaryan]

This deviates from how the other signals work. To make it consistent we could duplicate all tables into ExportProfilesServiceRequest but that seems too much duplication. I am not sure if there is a better way, so probably we should accept that ExportProfilesServiceRequest is not going to be consistent with other Request messages.

[dmathieu]

Would it make more sense to keep the ResourceProfiles as they are, drop ProfilesData and move all the lookup tables into ResourceProfiles?

[tigrannajaryan]

We made Data and Request distinct for the following reason: the Request is intended to be used for RPC calls, the Data is intended for local serialization. They are the same today but may evolve differently, e.g. we may add RPC-specific fields to the Request (see for example this PR that was not accepted, but gives an idea of what an RPC-specific field can be).

[dmathieu]

We can move the lookup tables into ResourceProfiles and keep ProfilesData for future use then.

[christos68k]

Can we add some of text from the proto here (in light of recent changes):

Suggested change

	// The ProfilesData.
	// For data coming from a single resource this object will typically contain
	// one element resource profiles. Intermediary nodes (such as OpenTelemetry
	// Collector) that receive data from multiple origins typically batch the
	// data before forwarding further and in that case there will be multiple
	// resources.
	// The ProfilesData which contains an array of ResourceProfiles.
	// For data coming from an SDK profiler, this array will typically contain one
	// element. Host-level profilers will usually create one ResourceProfile per
	// container, as well as one additional ResourceProfile grouping all samples
	// from non-containerized processes. Intermediary nodes (such as OpenTelemetry
	// Collector) that receive data from multiple origins typically batch the
	// data before forwarding further and in that case there will also be multiple
	// ResourceProfiles.

[aalexand]

We can move the lookup tables into ResourceProfiles and keep ProfilesData for future use then.

Moving the lookup tables into ResourceProfiles would mean they would be duplicated between the profile data for different containers. Sharing the lookup tables was the main goal of #644.

It looks like the current design constraints are:

1. FoosData messages are intended to be simple wrappers for an array of ResourceFoos. For all other signals the Data message simply has repeated field of the Resource messages.
2. The purpose of the Data messages is to support local serialization, they are not intended to be used in the RPCs.
3. ExportFooServiceRequest messages mirror the FoosData messages, but are intentionally separate to allow evolving them independently. Primarily to allow the Request messages to have fields that are specific to the RPC invocation but are not related to either local serialization, nor the data model itself.
4. For profiling, each resource represents a cgroup and sharing the data between resources via the lookup tables is important to save in-memory and wire space. This was the goal of profiles: Move lookup tables to ProfilesData #644.

The options appear to be:

1. Accept the proposal to use the Data message in the Request message. This breaks the consistency and conventions 2) and 3) above.
2. Mirror the lookup table fields in the Resource message. This sort of keep the consistency between different signals, but adds redundancy and will likely cause a bunch of boilerplate code related to dealing with these parallel fields.
3. Approach the problem of "How a signal can have some data that is shared between resources in a way that is consistent between signals and compatible with both local serialization and RPC conventions?" in some other way.

[jhalliday]

I'm inclined towards Alexey's #3 option...

Whilst I think we have good reasons for wanting profiles to deviate from the way other signal types work, I also think there are reusable patterns in our work that may flow back into future refinement of those older signal types or development of new ones. I'd like to find a clean way to express a pattern for more portability. For example

message ${Signal}ReferenceTables {
   repeated string string_table = 1;
   repeated Foo foo_table = 2;
   repeated Bar bar_table = 3;
   ...
}

message ${Signal}Data {
  repeated Resource${Signal} resource_${signal} = 1;
  ${Signal}ReferenceTables lookup_tables = 2;
}

message Export${Signal}ServiceRequest {
  repeated opentelemetry.proto.${signal}.Resource${Signal} resource_${signal} = 1;
  opentelemetry.proto.${signal}.${Signal}ReferenceTables lookup_tables = 2;
}

Any signal which may benefit from the space savings of shared reference data may define a signal-specific bundle of lookup tables as a message and if desired retrofit it in a backwards compatible manner to its existing _Data or Export_ServiceRequest messages.

Treating the reference tables as a separate building block may also allow for more abstract handling in tools or pipelines that wish to e.g. reduce a stream of (tables,datapoints),(tables,datapoints),... tuples by merging the tables and recomputing-in-place the datapoints to point to the unified tables, which seems likely to be a common and desirable space saving task, particularly for back-end ingest.
Likewise, and at risk of reopening the stateful vs stateless protocol debate, adding a unique identifier to a ${Signal}ReferenceTables structure would allow for a set of samples (e.g. message Profile) to explicitly say which tables message they are referring to rather than implicitly referring to the one in the same message/file heirarchy, opening the way for cases where a _Data or Export_ServiceRequest doesn't populate the tables field, but carries via the resources field, samples which refer to tables stored or transmitted in a separate Data or ServiceRequest instance.

Scope creep is a thing, but framing the change thus as an optional, flexible and compatible space saving addition to the existing Data/ExportRequest structures rather than a redefinition/repurposing/(polluting?) of them is perhaps a more interesting way of considering the proposal, particularly at TC level.

[aalexand]

I'm inclined towards Alexey's #3 option...

Whilst I think we have good reasons for wanting profiles to deviate from the way other signal types work, I also think there are reusable patterns in our work that may flow back into future refinement of those older signal types or development of new ones. I'd like to find a clean way to express a pattern for more portability. For example

...

Pulling out all lookup tables into a separate message makes sense.

As a naming-is-hard note: the proposal uses both "reference" and "lookup" terms, we should pick one and standardize on it across all signals. I think "reference" has too many other meanings, something like "lookup" or "dictionary" could be cleaner. But naming is hard of course.

[tigrannajaryan]

PUlling out all lookup tables into a separate message makes sense.

+1.

[dmathieu]

This is superseded by #650.