Add SDK data classes for experimental profiling signal type.

[jhalliday]

First small step towards supporting the new experimental signal type for profiling!

This PR introduces data interfaces corresponding to the OTLP model as defined in https://github.com/open-telemetry/oteps/pull/239 https://github.com/open-telemetry/opentelemetry-proto/pull/534

Until the latter is merged the wire format is still something of a moving target, but no major changes are expected so it should be safe to start reviewing this...

The code structure corresponds pretty much 1:1 with the .proto files, modulo some tidying up of field names for better readability. The easiest way to review is probably split screen the .java and .proto files - fields appear in the same order in both. The bulk of the work will likely be getting familiar with the new data model, the code itself its not complicated.

The javadocs are more or less copies from the .proto comments, many of which are in turn inherited from google's pprof.proto The OTel profiling SIG has an open task to better document the data model, after which another pass on the code will update the javadocs to match. For now they should be considered placeholders.

Some elements are @Deprecated. This is unusual for fresh code! The situation arises because the new OTel model builds on google's existing pprof one, adding some fields and deprecating others. In order to allow 'legacy' style data to be written for compatibility, the data classes have methods for the older fields. An alternative design choice is not to support these, which should in theory allow all necessary data to be written for sending to OTel receivers, but would not allow for older style data expected by existing pprof tools. I've opted for more flexibility since there is little cost.

The profiling format is I think the first OTLP spec in which byte[] type fields appear as first class citizens, not merely as carriers for strings. How to model this in the Java API is an open question, given the design preferences around nullability and immutability. ByteBuffer isn't ideal, but may be preferred to raw byte[]. Protobuf uses ByteString, but that's more relevant to the Marshalers than the API - we don't really want a dependency on protobuf runtime classes at this level.

This foundation code leads on to the Immutable data classes, then the Marshalers. Those are mostly ready, but I prefer smaller pieces for review, so they will follow in later PRs. The Marshalers in particular will require the new proto PR above to be merged first anyhow.

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: jhalliday / name: Jonathan Halliday (ae94404424ae45400d3d83e37d0d3e0ef09f6126, a7af9cb524c5d39928a7231efe7c5a2c78a7ddc4, 3044610b9f6ef77a4fdcf898dca211ce165584bb, f84df9868356dce19eb25e672208f4bbb3b8b3f3, 64da240d57c0899aaa64313a6995ab9776dd1e7d, a84e2f92343671ef3c0ba0ea795cd4a5ece95662, ffb73b978b401f485318b64f21b5173befd430bc, 112796bfa7aa67c05520fa2c161f0653cc2e28ef, bda144f32ec5e250a2ca2e40e094ffc49f353897, 087ce897f4a56390258881d740d8e32511afd482, 2fa0060082a0ea67be9b9b3ba70320854388ad7f, 55b8ab6b4ebff7126239ad705802044dd248bb53, 4d31472768646f4228d58a27035fceedb9fc479d, d6e795173d552fb8fdc799f8f8208d96126d639e)

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 90.86%. Comparing base (66cf1b6) to head (55b8ab6). Report is 66 commits behind head on main.

Files	Patch %	Lines
...exporter/otlp/profiles/AggregationTemporality.java	0.00%	3 Missing :warning:
...ntelemetry/exporter/otlp/profiles/BuildIdKind.java	0.00%	3 Missing :warning:
...y/exporter/otlp/profiles/ProfileContainerData.java	0.00%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6374      +/-   ##
============================================
- Coverage     91.09%   90.86%   -0.24%     
- Complexity     5772     6168     +396     
============================================
  Files           627      681      +54     
  Lines         16852    18491    +1639     
  Branches       1720     1813      +93     
============================================
+ Hits          15351    16801    +1450     
- Misses         1006     1154     +148     
- Partials        495      536      +41     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jkwatson]

I'm not sure that mirroring the protos exactly is a great way to build out our SDK classes. We have the opportunity to build something better than raw protos that will apply specifically to java. I'd want to have a very strong reason to expose clearly mutable data (for example), and yet call it as Immutable.

[jhalliday]

I'm not sure that mirroring the protos exactly is a great way to build out our SDK classes. We have the opportunity to build something better than raw protos that will apply specifically to java. I'd want to have a very strong reason to expose clearly mutable data (for example), and yet call it as Immutable.

I think there are perhaps two separate issues here. I'll take them in reverse order:

I don't like byte[] in the API and don't expect it to be used in the final version, but before I can swap it for something better we'll need to agree on what that is. ByteBuffer would do, but isn't reliably immutable either. Then again nor is List, which is widely used in the API anyhow. At least it solves the Nullability problem which is distinct from the Immutability one.

The 1:1 mapping to the proto is absolutely horrible as a user API and I am sure it's not a great way to build such an API. That said, this isn't intended to be the user facing API. It's intended for feeding the Marshalers which encode the proto, and for that 1:1 produces the cleanest, least error prone code and maximises flexibility. As it gets more usage, we'll likely start finding patterns that are more user friendly and I expect the eventual public (as distinct from internal) API will look different. Designing that, I'd be concerned with masking the lookup table mechanism, so users can e.g. feed in a List<StackFrame[]> and have all the decomposing of that handled for them. That's likely to take the form of an SDK package that maps the user facing layer onto these Marshaler-facing internal data classes. It's possible that putting these interfaces where I have in the repository structure is misleading as to their intended use, but then again all other protos have more or less 1:1 structure like this, so from a uniformity and maintenance point of view it follows principle of least surprise. Profiling is perhaps the first proto where the wire structure is so significantly different from the optimal user API, so using the same classes for both purposes, as in other signal types, won't work so well. We'll just have to feel out what that means for the package structure as we go along I think.

[jkwatson]

If our goal is to provide a route into marshallers, maybe we should create a write-only API that facilitates this use-case, rather than constructing a proto-mirroring set of Java classes. 🤔

Let's discuss approaches at the SIG meeting this week.

[jhalliday]

If our goal is to provide a route into marshallers, maybe we should create a write-only API that facilitates this use-case

Marshallers read an object model and transform it into wire format data. This is that object model. It can't be write-only, because then the marshallers have nothing to read. It should be 1:1 with the wire format, so the marshallers don't have to handle complex transformations. This API is well designed for its intended use case. If you have a different use case, that's a different API.

[jkwatson]

If our goal is to provide a route into marshallers, maybe we should create a write-only API that facilitates this use-case

Marshallers read an object model and transform it into wire format data. This is that object model. It can't be write-only, because then the marshallers have nothing to read. It should be 1:1 with the wire format, so the marshallers don't have to handle complex transformations. This API is well designed for its intended use case. If you have a different use case, that's a different API.

If our Marshaller exposed a "write only API" to the producer of the profile, then the marshaller can do with the data as it will. There doesn't need to be an intermediate representation, necessarily.

[jhalliday]

If our Marshaller exposed a "write only API" to the producer of the profile, then the marshaller can do with the data as it will. There doesn't need to be an intermediate representation, necessarily.

True. However, all existing Marshallers, including the ones I already wrote for profiling, work the same way. This follows principle of least astonishment, ensuring that users/maintainers can reuse prior knowledge of the pattern when working with the new classes. It seems to me there would need to be a good reason for deviating from that. What is it?

[jkwatson]

If our Marshaller exposed a "write only API" to the producer of the profile, then the marshaller can do with the data as it will. There doesn't need to be an intermediate representation, necessarily.

True. However, all existing Marshallers, including the ones I already wrote for profiling, work the same way. This follows principle of least astonishment, ensuring that users/maintainers can reuse prior knowledge of the pattern when working with the new classes. It seems to me there would need to be a good reason for deviating from that. What is it?

The primary reason is highlighted by the proposal in this PR. Having an intermediate representation as ugly and non-ergonomic as this one seems like a very good reason to do things differently. The profiling signal is several orders of magnitude (at least) more complex than either metrics, logs or spans. I think it's worth considering a radically different approach.

A big question to answer: how many exporters are there going to be, realistically? If it's just OTLP, then I think skipping the intermediate representation would allow optimizations that would not be possible if it would be required to convert back and forth.

[jhalliday]

In term of relocating this to the same place the marshallers will eventually be, how about: exporters/otlp/profiles/src/main/java/io/opentelemetry/exporter/otlp/profiles/data or exporters/profiles-otlp/src/main/java/io/opentelemetry/exporter/profiles/otlp/data

[jack-berg]

We don't have any other signals that are structured purely as an exporter, with exporter and data classes bundled together without any sdk artifact.

Looking at the opentelemetry-exporter-otlp artifact, we see packages:

• io.opentelemetry.exporter.otlp
  • http
    • logs - otlp/http logs
    • metrics - otlp/http metrics
    • traces - otlp/http traces
  • logs - otlp/grpc logs
  • metrics - otlp/grpc metrics
  • traces - otlp/grpc traces

And over in the sdk packages (say opentelemetry-sdk-traces for examples), we see packages:

• io.opentelemetry.sdk.traces - top level trace sdk
  • data - trace data interfaces
  • export - trace exporter interfaces
  • samplers - sampler interfaces

(Side note - we could have done better with the SDK package design. Not sure what we gain by having dedicated packages for data, exporting, and samplers. But what's done is done.)

Can we preserve parts of both of these package structures coherently in a single OTLP profile artifact? Not exactly. The two package hierarchies diverge so we couldn't have a single root package for the java module system. Since this is an exporter artifact and not an SDK artifact, let's prioritize symmetry with the existing OTLP exporter artifact:

• io.opentelemetry.exporter.otlp
  • http
    • profiles - otlp/http profile
  • profiles - otlp/grpc profile
  • data - profile data interfaces
  • export - profile exporter interfaces

I don't anticipate this being permanent, since by bundling data / exporter interfaces with the OTLP exporter, we limit any future exporter to take a dependency on the OTLP profile dependency. Minimally, I think we'd also be interested in a logging exporter at some point. But let's wait on that. Since the OTLP profile exporter will be the only thing dependent on the profile data / exporter interfaces, breaking them out into a separate sdk module will be minimally disruptive.

[jhalliday]

I think the lack of being able to reference attributes by index is a problem, but otherwise this seems like a good start.

Yes, I think we've reached the point where we should probably just accept that there are imperfections that will need some adjustment as we start to use the interfaces (I'm already having some doubts on ByteBuffer...) and deal with those as we go along and have a bit more experience with how it works in practice. Thanks for the reviews.