Clarify and potentially remove InstrumentationLibrarySpans about opentelemetry-proto HOT 12 CLOSED

Related to #148

from opentelemetry-proto.

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)

from opentelemetry-proto.

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.

from opentelemetry-proto.

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.

from opentelemetry-proto.

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.

from opentelemetry-proto.

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. :-)

from opentelemetry-proto.

@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.

from opentelemetry-proto.

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.

from opentelemetry-proto.

@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 

from opentelemetry-proto.

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.

from opentelemetry-proto.

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.

from opentelemetry-proto.

Closing, the decision is to keep InstrumentationLibrary.

from opentelemetry-proto.