Remove InstrumentationLibrarySpans and InstrumentationLibraryMetrics

[tigrannajaryan]

Resolves #149
Resolves #148

Problem

Traces

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

To illustrate the data structure that was needed before this commit,
here is an example:

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

See below what the data structure becomes after implementing the proposed
solution.

Metrics

The metrics proto currently includes InstrumentationLibraryMetrics which does
not have clearly defined semantics. InstrumentationLibraryMetrics may contain
a number of metrics all associated with one 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 metric identity. For example if I have 2 different
InstrumentationLibrarys each having a different name and both containing a
Metric that have the same MetricDescriptor.name are these 2 different
timeseries or the same one?

To illustrate the data structure that was needed before this commit,
here is an example:

resource_metrics:
  resource:
    ...
  instrumentation_library_metrics:
    - instrumentation_library:
        name: io.opentelemetry.redis
      metrics:
        - metric_descriptor:
            name: request.count
          int64_data_points:
            - value: 10

    - instrumentation_library:
        name: io.opentelemetry.apache.httpd
      metrics:
        - metric_descriptor:
            name: request.count
          int64_data_points:
            - value: 200

See below what the data structure becomes after implementing the proposed
solution.

Solution

Traces

This commit removes InstrumentationLibrarySpans message type from the protocol.
We will add semantic conventions for recording instrumentation library in Span
attributes.

The benefits of this approach over using InstrumentationLibrarySpans 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.

After removing InstrumentationLibrarySpans concept we have this data structure:

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

Once this commit is merged language SDKs will need to make a corresponding change
and add "instrumentation.library.name" (or whatever name is accepted in semantic
conventions) to Span attributes automatically.

Metrics

This commit removes InstrumentationLibraryMetrics message type from the protocol.
We will add semantic conventions for recording instrumentation library in Span
attributes.

Semantically the name of InstrumentationLibrary is equivalent to a metric label
so we will use metric labels to record the library name and version.

The benefits of this approach over using InstrumentationLibraryMetrics 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 metrics but don't care about instrumentation library concept (likely the
  majority of codebases).

• It uses the general concept of metric labels that already exists and is well
  understood and by doing so makes the semantics of instrumentation library name
  clear. The instrumentation library name is one of the labels that form
  timeseries identifier.

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

After removing InstrumentationLibraryMetrics concept we have this data structure:

resource_metrics:
  resource:
    ...
  metrics:
    - metric_descriptor:
        name: "request.count"
        int64_data_points:
          - value: 10
            labels:
              - key: instrumentation.library.name
                value: io.opentelemetry.redis

    - metric_descriptor:
        name: "request.count"
        int64_data_points:
          - value: 200
            labels:
              - key: instrumentation.library.name
                value: io.opentelemetry.apache.httpd

[tigrannajaryan]

Note that this is a breaking change to protocol. Since Traces are in Beta and Metrics are in Alpha we are still allowed to do this.

[jmacd]

I believe that instrumentation library belongs in Resources. The structure of a OTLP report ought to be:

resource:
  ... (includes instrumentation library)
  metrics:
    - metric_descriptor:
      ...
    - metric_descriptor:
      ...
  spans:
    - name: ...
       ...
    - name: ...
      ...

[jmacd]

Also repeating myself, I support multiple Resources per process: open-telemetry/oteps#78

[tigrannajaryan]

I believe that instrumentation library belongs in Resources. The structure of a OTLP report ought to be:

resource:
  ... (includes instrumentation library)
  metrics:
    - metric_descriptor:
      ...
    - metric_descriptor:
      ...
  spans:
    - name: ...
       ...
    - name: ...
      ...

@jmacd I do not have a strong opinion about where that instrumentation library of information should go. Once this PR is accepted the protocol will allow it to be recorded either in the Resource attributes or in Span attributes based on how we specify it in the semantic conventions. It can be discussed in the proposal for "instrumentation library" semantic convention and does not contradict this PR.

Structurally what you described matches very closely what we will have after this PR is merged. The only difference is that we do not allow spans and metrics in a single request. Traces and Metrics are currently separate requests. I do not think we need to put them in one request, but if you want to propose it then it can be done separately since that change is orthogonal to what this PR proposes.

Also repeating myself, I support multiple Resources per process: open-telemetry/oteps#78

This is already supported by the proto. Nothing prevents the SDKs to emit telemetry for multiple Resources per process. See ExportTraceServiceRequest and ExportMetricsServiceRequest message definitions.

[bogdandrutu]

Re #150 (comment)

I think resource has a very nice scoping property that identifies the application/service/process that produces the telemetry which will be lost if we do the merging you propose.

In order to not lose that property we can propose that instrumentation library is a separate part in the resource (properties for app + properties for instrumentation library) but would keep them separated to preserve the scoping aspect of the properties.

That being said it is a very long conversation about how scoping would be represented in the API (@jmacd you have a proposal) but I would like to make sure that at least we preserve the scoping in the protocol. So it may be a good moment to identify these scopes (not proposing to change the API) in order to make sure we support them in the protocol.

[jmacd]

This comment is unrelated to my previous comment. If we turn the instrumentation library into just another span attribute or metric label, we now have the situation where multiple instruments have exactly the same name, where the only differentiator is the library name. This violates the principle we discussed yesterday in the Metrics SIG for when to choose labels vs extending a metric name. The Prometheus documentation on this states the following: "As a rule of thumb, [aggregations] over all the dimensions of a given metric should be meaningful". Applying instrumentation library as a label violates this rule.

[jmacd]

I think it is dangerous to think of the Resource as being limited to application/service/process. I would like to be able to write a virtual server that emits spans for different applications, services, and processes. In those cases, I'll place those attributes into the Span, not the Resource.

[tigrannajaryan]

This comment is unrelated to my previous comment. If we turn the instrumentation library into just another span attribute or metric label, we now have the situation where multiple instruments have exactly the same name, where the only differentiator is the library name. This violates the principle we discussed yesterday in the Metrics SIG for when to choose labels vs extending a metric name. The Prometheus documentation on this states the following: "As a rule of thumb, [aggregations] over all the dimensions of a given metric should be meaningful". Applying instrumentation library as a label violates this rule.

Good to know, I was not aware of this. However, it still does not change the reasons behind this proposal. It may change how "instrumentation library" should be translated to a Metric, but does not change the fact that "instrumentation library" is a superfluous concept in the protocol.

What do we do today when exporting to Prometheus from SDKs? Where does instrumentation library name go? Does it become a label or it becomes part of the metric name? If Prometheus's recommendations are the model to follow then we can follow the same recommendations when exporting to OTLP.

Again, I am not aware of any special facility in Prometheus or any other metric protocol that allows to record "instrumentation library" in any special distinct way. You either have to make it a label or put in a metric name (is there any other option?). Whatever decisions are made regarding "instrumentation library" when exporting from SDKs to Prometheus or other metric protocols should apply to OTLP as well.

I do not see the point of adding a new concept to the protocol if every other protocol and every existing backend in the industry does not understand that concept. We have to translate this to understandable concepts at some point in collection pipeline anyway. If we refuse to make this translation in the SDKs we are simply moving the problem to the Collector, which is pointless because we have to solve this problem in SDKs anyway for all other protocols except OTLP. If we have to solve this problem in SDKs then I do not see why the solution should not be uniformly applied to all metric protocols, including OTLP.

I may be very wrong in my knowledge of metric industry and protocols, so please correct me. Happy to retract this PR if there is a contradicting evidence.

[jmacd]

@bogdandrutu I think this should be your call. I agree with @tigrannajaryan that these values can be treated as either resources or labels.

[jmacd]

@bogdandrutu I was waiting for your opinion, but I'm willing to support this proposal.

[tigrannajaryan]

I put a bit more thought into this.

One way to think about this is that instrumentation library name and version
specify a location that emits the telemetry. What other locations are possible?
Here is are some examples:

Attribute	Explanation
Service Name	Logical name of the service.
Service Version	The version number of the service.
Commit Id	The commit hash that identifies the source code from which the service is built.
Container Image	Name of the container image.
Executable Name	The name of the executable file that implements the service.
Module	The name of the source-code module (e.g. Go module).
Package	Go or Java package name.
File	Source code file name.
Class	Class name of the implementation.
LineNo	Line number in the source code.
Lambda Function ARN	The ARN of AWS Lambda function.
Service Instance ID	The ID of the running service instance.
Pod ID	Kubernetes pod ID.
Node ID	Kubernetes node ID where the pod is running.
Process ID	Process ID of the running service.
Hostname	The host name where the service is running.
VM ID	Virtual machine ID.
Container ID	Running container ID.
Cloud Provider	Cloud provider where the VM is located.
Cloud Region	The specific region of the cloud provider where VM is located.
Lambda Execution ID	The execution ID of a particular invocation of AWS Lambda.

Side note: if we you look carefully we see that some of these are defined at
build time, some others at runtime (hint: I listed build-time attributes first).
Instrumentation library name and version specify a location that is defined at
build time.

For many of these we already have semantic conventions to store them in the
Resource attributes. It is not clear why Instrumentation library deserves as
special treatment in protocol while many others are not.

I believe instrumentation library name and version is nothing but another
attribute that defines where the telemetry is emitted. And for that we already
have a concept: the Resource.

API Implications

Incidentally, I believe our API needs to be modified to allow recording other
location attributes more easily.

The API currently provides a way to obtain a Tracer or a Meter by providing a
name and a version. The telemetry emitted via the obtained object will then
include the provided name and version as the Instrumentation library name and
version.

The API does not define a way to specify the Resource that is emitted. The
specification says that the Resource will be populated by the SDK and the
information that will be included in the resource can be automatically detected
by the SDK based on the information available to the SDK at runtime or will be
provided to the TracerProvider or MeterProvider when the provider is created.

This is unfortunately insufficient since it mandates that the Resource is
defined via SDK calls, and the SDK normally is accessible only from the intended
single point of SDK configuration somewhere at the application startup code. It
thus does not allow parts of the application source code that only have access
to the API to specify information to be included in the Resource.

I suggest to extend the API to allow to create a Resource with specified
build-time or runtime attributes. We also extend the API to allow to obtain a
Tracer or a Meter with the specified Resource:

resource := resource.Merge(
    New(kv.String("instrumentation.library.name", "io.opentelemetry.contrib.mongodb")),
    global.Resource())
tracer := global.TracerWithResource(resource)

Alternatively the API may force that the provided Resource is always merged with
the global Resource, i.e.:

resource := New(kv.String("instrumentation.library.name", "io.opentelemetry.contrib.mongodb")))
tracer := global.TracerWithResource(resource) // The `resource` will be merged with global.Resource().

For backward compatibility purposes (or if we feel that this is the most common
use case for obtaining a Tracer or Meter) the existing API that allows obtaining
the Tracer or Meter by providing the name and version of the instrumentation
library will stay. The existing API can be implemented as a specialized case
that calls the more generic API and provides instrumentation library name and
version as key-value pairs, e.g.:

func Tracer(name string) trace.Tracer {
    resource := resource.Merge(
        New(kv.String("instrumentation.library.name", name)),
        global.Resource())
    return TracerWithResource(resource)
}

The instrumenting libraries or other components of the source code will
typically use the new API to provide build-time attributes of the Resource
(although nothing prevents them from providing runtime attributes as well).

This modification allows instrumenting libraries to record additional
attributes. For example it will be valuable to record the instrumented library
name in addition to the instrumenting library name, which today is not
directly possible.

It also opens up the possibility for other components of the source code to
record attributes. For example a Tracer obtained by a Java class may include the
class name in the Resource attributes. In some other cases the Tracer may be
associated with a Resource that contains attributes that describe the module,
package, file name and other build-time attributes.

Future possibilities

It is interesting to note that built-time attributes of the Resource often form
a hierarchy. For example the service may contain multiple libraries (a the
library may contain multiple files, etc).

The telemetry emitted by Tracers obtained for different libraries that belong to
the same service will have Resources where attributes that describe the service
contain duplicate information:

Resource:
    service.name: A
    service.version: "semver:2.0.0"
    service.instance.id: 627cc493-f310-47de-96bd-71410b7dec09
    k8s.pod.name: P1
    instrumentation.library: io.opentelemetry.contrib.mysql
Spans:
    - start_time: 2020/01/01 00:00:00.000
      end_time: 2020/01/01 00:00:00.050
      db.statement: "SELECT * FROM user_table"
Resource:
    service.name: A
    service.version: "semver:2.0.0"
    service.instance.id: 627cc493-f310-47de-96bd-71410b7dec09
    k8s.pod.name: P1
    instrumentation.library: io.opentelemetry.contrib.redis
Spans:
    - start_time: 2020/01/01 00:00:01.000
      end_time: 2020/01/01 00:00:01.020
      db.statement: "SET mykey 'Value'"

Here we see that 4 out of 5 attributes in the 2 different Resources are the
same. If we could avoid transmitting the same attributes of again and again we
would improve the efficiency.

This is possible do by making a small, backward-compatible modification to OTLP
schema. We can define ResourceSpans the following way (and ResourceMetrics
similarly):

message ResourceSpans {
    Resource resource = 1;
    repeated Span spans = 2;
    // This is a new field added in backward compatible manner.
    repeated ResourceSpans nested_resource_spans = 3;
}

The new nested_resource_spans field recursively refers to its containing
message type and allows to model a hierarchy of Resources, each containing a
list of Spans. The above example with instrumentation libraries will be recorded
as:

Resource:
    service.name: A
    service.version: "semver:2.0.0"
    service.instance.id: 627cc493-f310-47de-96bd-71410b7dec09
    k8s.pod.name: P1
    NestedResourceSpans:
        - Resource:
            instrumentation.library: io.opentelemetry.contrib.mysql
          Spans:
            - start_time: 2020/01/01 00:00:00.000
              end_time: 2020/01/01 00:00:00.050
              db.statement: "SELECT * FROM user_table"
        - Resource:
            instrumentation.library: io.opentelemetry.contrib.redis
          Spans:
            - start_time: 2020/01/01 00:00:01.000
              end_time: 2020/01/01 00:00:01.020
              db.statement: "SET mykey 'Value'"

Such hierarchical representation complicates the processing of telemetry data
and should be only adopted if there is a good evidence that it is necessary.

If this hierarchical representation is adopted in OTLP then in the SDK we do
not necessarily need to implement Resource Merge function in a way that performs
actual merging of attributes. Merge function may preserve the hierarchy of
Resources and use that information to emit more compact hierarchical
representation in OTLP (while flattening and merging the attributes when
exporting in other formats).

Note that to make operations on hierarchical representation possible we need to
have the following semantics: if the same attribute exists on a Resource on a
child level and on a containing level then the attribute value at the child
level overrides the attribute value at the containing level Resource.

[jmacd]

extend the API to allow to create a Resource with specified
build-time or runtime attributes

I support this! I had written a similar proposal to support "Resource Scope":
open-telemetry/oteps#78

[jmacd]

The new nested_resource_spans field recursively refers to its containing
message type and allows to model a hierarchy of Resources, each containing a
list of Spans.

I had made a similar proposal about nesting in OTLP Metrics, because when we introduce metric exemplars there are likely to be common labels that could be nested as you have done: open-telemetry/oteps#113 (comment)

I asked a some coworkers with experience in metrics protocols, and they responded that OTLP should find a way to de-duplicate repetitive labels and attributes by moving them into a dedicated section. Instead of nesting, every distinct set of labels could be listed in this dedicated area, and Spans and Metrics would refer to them by an index into that section. This approach is less recursive, and it requires the implementation to have a way of deduplicating label sets. This is something the metrics pipeline is already doing, so it would be fairly straightforward.

[tigrannajaryan]

@open-telemetry/specs-approvers please review.

[bogdandrutu]

I am not convinced that we adopt the correct design here, as I initially proposed in open-telemetry/oteps#84 I can see that there are 3 levels/scopes of information that we want to record:

1. Attributes/data associated with the process/binary.
2. Attributes/data associated with a component inside the process/binary.
3. The signal itself (span/metric/log/etc.) which correspond to a request in case of a span/log or an aggregation of requests in case of metrics, but they have as a primary scope the "request"

I got a lot of push back about calling the second scope component because people mentioned to me that the instrumentation library does not represent the component, but the instrumentation library that does instrument the component (which I get, but I still do think that it describes the component inside the binary/process). There are other things that we may want to have in the second scope like properties of that specific instance of the component (I think @jmacd mentioned things like database connection address, etc.)

So my preference would be to go with the same levels we currently have (calling them both resource as you proposed, or resource + component) and have a clear scope for the attributes/data would clarify the protocol (again personal opinion), the current proposed solution does remove the scoping of the attributes/data which worries me because it makes things complicated on the consumer side of the data to identify which data correspond to the process vs which data correspond to a component inside the process.

[jmacd]

to identify which data correspond to the process vs which data correspond to a component inside the process

I am starting to see this desire -- "process identification" -- as the problem here. I don't believe there's one good answer, and would prefer if we think of "how to identify the process" as a concern of the query and the monitoring system. Who should decide what is and is not a process? I think the operator should make this decision, not the developer.

There are some conventional descriptions of "what is a process", but they are not unique. How should I describe a process of a runtime that forks a new process for each request? How about this: I am performing a user-specific analysis and for the purpose of this one particular query, I want to consider each one of my users "a process". I think we should state that semantically, an attribute is a label is a resource, they're just metadata. How we interpret the metadata decides what is and is not a process.

[tigrannajaryan]

OK, we had some more live discussions together with @bogdandrutu and the conclusion is that it is not entirely clear that this proposal is what we want to end up with in the long term.

Neither the complete removal of InstrumentationLibrary nor the alternate approached that uses NestedResourceSpans qualify as undoubtfull improvements over the current state.

We are still evolving our understanding of how exactly we want to represent the Resource or its parts (e.g. the various build-time and runtime attributes). There is not enough clarity on this to make a precise and future-proof schema proposal right now.

Given that I believe it is best that we keep the current state as is for now. It is a good enough representation of the telemetry data that can serve us well for at least short and midterm which we can call V1 of the protocol/schema. Once we have a significantly better understanding we may make a proposal for a different schema which can be labeled as V2.

[tigrannajaryan]

Based on the above I am closing this PR for now. Any further changes can be proposed in a new PR.