Clarify and potentially remove InstrumentationLibrarySpans

[tigrannajaryan]

The traces proto currently contains InstrumentationLibrarySpans which does
not have clearly defined semantics. InstrumentationLibrarySpans may contain a
number of spans all associated with an InstrumentationLibrary. The nature of
this association is not clear.

The InstrumentationLibrary has a name and a version. It is not clear if
these fields are part of Resource identity or are attributes of a Span.
Presumably they should be interpreted as attributes of the Span.

I am not aware of any other trace protocols or backends that have the
equivalent of InstrumentationLibrary concept. However ultimately all span data
produced by OpenTelemetry libraries will end up in a backend and the
InstrumentationLibrary concept must be mapped to an existing concept. Span
attributes seem to be the only concept that fit the bill. Using attributes from
the start of the collection pipeline removes the need to deal with
InstrumentationLibrary by all codebases that need to make a mapping decision
(Collector, backend ingest points, etc).

Proposal 1

I suggest to remove InstrumentationLibrary message type from the protocol and
add semantic conventions for recording instrumentation library in Span
attributes.

The benefits of this approach over using InstrumentationLibrary are the following:

• There is not need for a new concept and new message type at the protocol
  level. This adds unnecessary complexity to all codebases that need to read and
  write traces but don't care about instrumentation library concept (likely the
  majory of codebases).

• It uses the general concept of attributes that already exists and is well
  understood and by doing so makes the semantics of instrumentation library name
  clear.

There is potentially a small downside: using InstrumentationLibrary message
may be more efficient to encode/decode especially especially if there are
multiple spans from the same instrumentation library (see Proposal 2 for a
potential solution to this).

On the other hand, for Span data that does not have an associated
instrumentation library name and version we are incurring cost of having the
InstrumentationLibrarySpans message even if it does not refer to any
InstrumentationLibrary. We pay the cost of the concept even if we don't need
it. As opposed to this the approach with recording instrumentation library
information in attributes using semantic conventions is zero cost for cases
which do not need to record instrumentation library.

To illustrate, here is an example. Using InstrumentationLibrarySpans we would
need to record the following:

resource_spans:
  resource: 
    ...
  instrumentation_library_spans:
    - instrumentation_library:
        name: io.opentelemetry.redis
      spans:
        - name: request
            start_time: 123

    - instrumentation_library:
        name: io.opentelemetry.apache.httpd
      spans:
        - name: request
            start_time: 456

After removing InstrumentationLibrarySpans concept we would do this:

resource_spans:
  resource: 
    ...
  spans:
    - name: request
      start_time: 123
      attributes:
        - key: instrumentation.library.name
          value: io.opentelemetry.redis

    - name: request
      start_time: 456
      attributes:
        - key: instrumentation.library.name
          value: io.opentelemetry.apache.httpd

Proposal 2

Rename InstrumentationLibrarySpans to SpanBatch and instead of allowing to
only record instrumentation library name and version allow recording any span
attributes at the batch level:

resource_spans:
  resource: 
    ...
  span_batches:
    - common_attributes:
        - key: instrumentation.library.name
          value: io.opentelemetry.redis
      spans:
        - name: request
            start_time: 123

    - common_attributes:
        - key: instrumentation.library.name
          value: io.opentelemetry.apache.httpd
      spans:
        - name: request
            start_time: 456 

This allows to represent the instrumentation library or anything other
attributes that are common for a batch of spans. The protocol will require that
common_attributes and Span aatributes do not contain the same key to avoid
expensive merging in translations.

The benefits of this approach is that instead of having a special concept just
for instrumentation library we have a generic mechanism to record repeating span
attributes efficiently (and instrumentation library information is just one of
the use cases).

Similar approach will also work nicely for log attributes when we add log
support.

[tigrannajaryan]

Related to #148

[SergeyKanzhelev]

I very much like proposal 2 as it allows simplified SDKs which just send everything as-is as well as optimized upload. I would extend it however to not only common attributes, but also common resources. Like mentioned here: #94 (comment)

[tigrannajaryan]

Common resources are already possible in the protocol. A list of spans is currently placed under a ResourceSpans which is associated with a single Resource. That single resource is common for all spans in that list.

[bogdandrutu]

The InstrumentationLibrary has a name and a version. It is not clear if
these fields are part of Resource identity or are attributes of a Span.
Presumably they should be interpreted as attributes of the Span.

This is exactly the point is a separate thing, and not an attribute or a resource, it is a different scope inside the Resource.

[SergeyKanzhelev]

Common resources are already possible in the protocol. A list of spans is currently placed under a > ResourceSpans which is associated with a single Resource. That single resource is common for all > spans in that list.

The main point of InstrumentationLibrary type is that in most cases every span will have the same Resource, but library fields which used to be on Resource object as attributes were different. Thus the idea was to keep common Resource and add another level of commonality via InstrumentationLibrary.

There are cases though when a single process will host multiple apps. For this scenario having another level of resource span batch may be very efficient.

[tigrannajaryan]

This is exactly the point is a separate thing, and not an attribute or a resource, it is a different scope inside the Resource.

@bogdandrutu Sorry, this is the part I am struggling to understand.

I understand that InstrumentationLibrary has conceptual counterparts in the API because it makes API nicer to use (getting named Tracer and using it). However why do we need to preserve this concept on the wire? Why can it not be just an attribute when it leaves the SDK?

We actually have to convert it to an attribute when when SDK is configured to use Jaeger, Zipkin or any other exporter. We have to do it because none of trace protocols has the InstrumentationLibrary concept at the protocol level. Neither does any backend have such concept.

If this is true I am failing to see why InstrumentationLibrary deserves a dedicated place in OpenTelemetry protocol.

Let me quote from protocol Design Goals document:

Be suitable for use between all of the following node types: instrumented applications, telemetry backends, local agents, stand-alone collectors/forwarders.

I find that the current design that uses InstrumentationLibrary is skewed towards the needs of OpenTelemetry SDK at the cost of making the life of other node types more complicated. None of the other nodes benefit from the existence of InstrumentationLibrary, it just adds more complexity, which is avoidable.

I see the evidence of that complexity in all translation functions in the Collector. And we even have to add a separate processor type to handle filtering by instrumentation library as shown by this newly submitted PR which would be unnecessary if instrumentation library was a regular attribute.

I agree with you that in OpenTelemetry API/SDK it is a different concept but I think that it would be nice to contain that difference in the boundaries of API/SDK and not expose all other components of collection pipeline to this concept and force them to understand and handle it.

Please let me know what you think, I may be missing something important. Perhaps worth discussing this live. :-)

[SergeyKanzhelev]

@tigrannajaryan I fully support your position. I saw this concept an optimization layer and I like how this optimization can be replaced by span batches (your proposal 2). The only suggestion is to include resource attributes into SpanBatch as well. We can also decide to implement span batches later if needed.

[bogdandrutu]

The only suggestion is to include resource attributes into SpanBatch as well.

I am sorry but I think this removes the entire benefit of having a Resource. The whole idea is that there is a scope associated with these attributes, by having everything as attribute we lose an extremely important property related to the scope of the attributes:

1. The resource attributes are attributes that describe the application/process source of the telemetry.
2. The InstrumentationLibrary describes the component inside the application/process that records (which I think a lot of people have a confusion about, why this does not describe the component that produces the telemetry) the telemetry.
3. Span attributes are attributes that describe that are specific to that operation/span.

By merging all of them you lose a very important property of all these attributes, which will lead in lacking the ability to correlate telemetry between Resources for example.

I am a BIG -1 on removing the Resource from the protocol or adding extra informations to the Resource that do not describe the application/process that produces the telemetry.

I see the evidence of that complexity in all translation functions in the Collector. And we even have to add a separate processor type to handle filtering by instrumentation library as shown by this newly submitted PR which would be unnecessary if instrumentation library was a regular attribute.

That is not a new processor type, is extending the functionality for an already existing processor.

[SergeyKanzhelev]

@bogdandrutu I'm not suggesting to remove resources. Sorry if I wasn't clear. I suggest that we extend SpanBatch from proposal 2 to also include additional resource attributes that are common to all spans in the batch. Like this:

resource_spans:
  resource: 
    ...
  span_batches:
    - common_attributes:
        - key: instrumentation.library.name
          value: io.opentelemetry.redis
    - common_resource_attributes:
        - key: tenant
        - value: A
      spans:
        - name: request
            start_time: 123

    - common_attributes:
        - key: instrumentation.library.name
          value: io.opentelemetry.apache.httpd
    - common_resource_attributes:
        - key: tenant
        - value: B
      spans:
        - name: request
            start_time: 456 

[tigrannajaryan]

I put a bit more though into this and I am inclining towards the simplicity of Proposal 1. Unfortunately Proposal 2 creates problems of its own.

For example in Collector we have a processor which can update the value of a Span attribute if it exists. This is a one-line "update" operation if there is a single attribute map in the Span.

If we introduce the common_attributes this operation becomes a lot more complicated:

if common_attributes contains the key {
  delete common_attributes[key]
  insert span.attributes[key]
} else {
  update span.attributes[key]
}

This appears simple but have lots of logic of this sort in Collector, some written by outside contributors who are not necessarily experts in the codebase. It will be a proliferation of checks and if you forget branch you will have hard-to-catch subtle bugs.

I did benchmarking of the "Proposal 1" and it is about 6% slower than current schema that uses InstrumentationLibrary if you actually include "instrumentation.library.name" attribute in every span (assuming 10 spans per instrumentation library). "Proposal 2" has about the same speed as current schema.

As much as I wish for the highest performance in our protocol I do not think "Proposal 2" is worth the complexity and we should use simple approach in "Proposal 1". It is easy to understand, easy to work with and difficult to make mistakes. I believe it is the right tradeoff.

I am going to look into a bit more details of this and will add a comment if I find something important.

[tigrannajaryan]

Another issue that needs to be addressed when doing either Proposal 1 or 2 is what happens if the SDK sets "instrumentation.library.name" attribute and the user calls Span.SetAttribute API with the same key.

I believe the answer is that the behavior should be similar to what happens if the user calls Span.SetAttribute API twice with any other key. Spec defines that the second call overwrites the existing value. We want the same behavior here, user supplied "instrumentation.library.name" attribute value will prevail, which is reasonable and non-surprising behavior.

This behavior is easy to achieve in Proposal 1. SDK just needs to add "instrumentation.library.name" attribute to newly created Span data. I checked Go and Java SDK's, they both use maps for attributes, which makes this trivially achievable.

Achieving the same behavior is more complicated in Proposal 2, there is no such trivial way anymore due to the need to keep common attributes in addition to regular attributes, which is another argument in favour of Proposal 1.

I am going to submit a PR with Proposal 1 implemented. If we feel strongly about the need to have Proposal 2 done, it can be a follow up PR.