Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for arrays and maps for attribute values #157

Merged

Conversation

tigrannajaryan
Copy link
Member

Summary

This adds support for arrays and maps to attribute values, including support for
nested values.

This is a breaking protocol change.

Resolves: open-telemetry/opentelemetry-specification#376
Resolves: #106

Motivation

There are several reasons for this change:

This change introduces AnyValue. AnyValue can represent arbitrary numeric,
boolean, string, arrays or maps of values and allows nesting.

AttributeKeyValue now uses AnyValue to store the "value" part.

Note: below "Current" refers to the state of the "master" branch before this
PR/commit is merged. "Proposed" refers to the schema suggested in this
PR/commit.

Performance

This change has a negative impact on the performance when using canonical Go
ProtoBuf compiler (compared to current OTLP state):

BenchmarkEncode/Current/Trace/Attribs-8              	     813	   1479588 ns/op
BenchmarkEncode/Proposed/Trace/Attribs-8             	     417	   2873476 ns/op
BenchmarkEncode/OpenCensus/Trace/Attribs-8           	     162	   7354799 ns/op

BenchmarkDecode/Current/Trace/Attribs-8              	     460	   2646059 ns/op	 1867627 B/op	   36201 allocs/op
BenchmarkDecode/Proposed/Trace/Attribs-8             	     246	   4827671 ns/op	 2171734 B/op	   56209 allocs/op
BenchmarkDecode/OpenCensus/Trace/Attribs-8           	     154	   7560952 ns/op	 2775949 B/op	   76166 allocs/op

However, I do not think this is important for most applications. Serialization
CPU and Memory usage is going to be a tiny portion of consumed resources for
most applications, except certain specialized ones.

For the perspective I am also showing OpenCensus in the benchmark to make it
clear that we are still significantly faster than it despite becoming slower
compared to the current state.

More importantly, performance critical applications can use Gogo ProtoBuf
compiler (Collector does use it), which gains performance due to this change:

BenchmarkEncode/Current(Gogo)/Trace/Attribs-8        	    1645	    705385 ns/op
BenchmarkEncode/Proposed(Gogo)/Trace/Attribs-8       	    1555	    698771 ns/op

BenchmarkDecode/Current(Gogo)/Trace/Attribs-8        	     537	   2241570 ns/op	 2139634 B/op	   36201 allocs/op
BenchmarkDecode/Proposed(Gogo)/Trace/Attribs-8       	     600	   2053120 ns/op	 1323287 B/op	   46205 allocs/op

With Gogo compiler proposed approach uses 40% less memory than the current
schema.

After considering all tradeoffs and alternates (see below) I believe this
proposal is the best overall approach for OTLP. It is idiomatic ProtoBuf, easy
to read and understand, is future-proof to adding new attribute types, has
enough flexibility to represent simple and complex attribute values for all
telemetry types and can be made fast by custom code generation for applications
where it matters using Gogo ProtoBuf compiler.

Note: all performance measurements are done for Go implementation only (although
it is expected that other languages should exhibit somewhat similar behavior).

Alternates Considered

I also designed and benchmarked several alternate schemas, see below.

Adding array value to AttributeKeyValue

This is the simplest approach. It doubles down on the current OTLP protocol
approach and simply adds "array_values" field to AttributeKeyValue, e.g.:

message AttributeKeyValue {
  // all existing fields here.

  // A list of values. "key" field of each element in the list is ignored.
  repeated AttributeKeyValue array_values = 7;
}

This eliminates the need to have a separate AnyValue message and has lower CPU
usage because it requires less indirections and less memory allocations per
value. However, this is semantically incorrect since the elements of the array
must actually be values not key-value pairs, which this schema violates. It also
uses more memory than the proposed approach:

BenchmarkEncode/Proposed/Trace/Attribs-8             	     400	   2869055 ns/op
BenchmarkEncode/MoreFieldsinAKV/Trace/Attribs-8      	     754	   1540978 ns/op

BenchmarkDecode/Proposed/Trace/Attribs-8             	     250	   4790010 ns/op	 2171741 B/op	   56209 allocs/op
BenchmarkDecode/MoreFieldsinAKV/Trace/Attribs-8      	     420	   2806918 ns/op	 2347827 B/op	   36201 allocs/op

It will become even worse memory-wise if in the future we need to add more data
types to attributes. This approach is not scalable for future needs and is
semantically wrong.

Fat AnyValue instead of oneof.

In this approach AnyValue contains all possible field values (similarly to how
AttributeKeyValue is currently):

message AnyValue {
    ValueType type = 1;
    bool bool_value = 2;
    string string_value = 3;
    int64 int_value = 4;
    double double_value = 5;
    repeated AnyValue list_values = 6;
    repeated AttributeKeyValue kvlist_values = 7;
}
message AttributeKeyValue {
    string key = 1;
    AnyValue value = 2;
}

This results in significantly bigger AnyValue in-memory. In vast majority of
cases attribute values of produced telemetry are strings (see e.g. semantic
conventions for proof). Integer and boolean values are also used, although
significantly less frequently than strings. Floating point number, arrays and
maps are likely going to be diminishingly rare in the attributes. If we keep all
these value types in AnyValue we will pay the cost for all these fields although
almost always only string value would be present.

Here are benchmarks comparing proposed schema and schema with fat AnyValue and
using string and integer attributes in spans:

BenchmarkEncode/Proposed/Trace/Attribs-8             	     415	   2894513 ns/op	  456866 B/op	   10005 allocs/op
BenchmarkEncode/FatAnyValue/Trace/Attribs-8          	     646	   1885003 ns/op	  385024 B/op	       1 allocs/op

BenchmarkDecode/Proposed/Trace/Attribs-8             	     247	   4872270 ns/op	 2171746 B/op	   56209 allocs/op
BenchmarkDecode/FatAnyValue/Trace/Attribs-8          	     343	   3423494 ns/op	 2988081 B/op	   46205 allocs/op

Memory usage with this approach is much higher and it also will become worse as
we add more types.

AnyValue plus ExoticValue

This is based on fat AnyValue approach but rarely used value types are moved to
a separate ExoticValue message that may be referenced from AnyValue if needed:

message AnyValue {
    ValueType type = 1;
    bool bool_value = 2;
    string string_value = 3;
    int64 int_value = 4;
    ExoticValue exotic_value = 5;
}
message ExoticValue {
    double double_value = 1;
    repeated AnyValue array_values = 2;
    repeated AttributeKeyValue kvlist_values = 3;
}
message AttributeKeyValue {
    string key = 1;
    AnyValue value = 2;
}

While this improves the performance (particularly lowers memory usage for most
frequently used types of attributes) it is awkward and sacrifices too much
readability and usability for small performance gains. Also for the rare cases
it is slow and uses even more memory so its edge case behavior is not desirable.

Using different schema for log data type

I also considered using a different message definition for LogRecord attributes
and Spans. This would allow to eliminate some of the requirements that we do not
yet formally have for Span attributes (particularly the need to have maps of
nested values).

However, this does not help much in terms of performance, makes Span and
LogRecord attributes non-interchangeable and significantly increases the bloat
of code in applications that need to work with both Spans and Log records.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 8, 2020

CLA Check
The committers are authorized under a signed CLA.

@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-approvers please review. I am working on signing the CLA.

@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-approvers please review.

@bogdandrutu
Copy link
Member

@open-telemetry/specs-approvers would like to merge this soon. Please approve/comment

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, aside from concern with using Attribute prefix for generic key-value types

@tigrannajaryan
Copy link
Member Author

@yurishkuro PTAL.

@tigrannajaryan
Copy link
Member Author

@open-telemetry/specs-approvers I need one more review/approval please.

## Summary

This adds support for arrays and maps to attribute values, including support for
nested values.

This is a breaking protocol change.

Resolves: open-telemetry/opentelemetry-specification#376
Resolves: open-telemetry#106

## Motivation

There are several reasons for this change:

- The API defines that attributes values [may contain arrays of values](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-attributes).
  However the protocol has no way of representing array values. We need to add
  such capability.

- We intend to support Log data type in the protocol, which also requires array
  values (it is a Log Data Model requirement). In addition, Log data type
  requires support of key-value lists (maps) as attribute values, including
  nested values.

- There are long-standing requests to support nested values, arrays and maps for
  attributes:
  open-telemetry/opentelemetry-specification#376
  open-telemetry/opentelemetry-specification#596

This change introduces AnyValue. AnyValue can represent arbitrary numeric,
boolean, string, arrays or maps of values and allows nesting.

AttributeKeyValue now uses AnyValue to store the "value" part.

Note: below "Current" refers to the state of the "master" branch before this
PR/commit is merged. "Proposed" refers to the schema suggested in this
PR/commit.

## Performance

This change has a negative impact on the performance when using canonical Go
ProtoBuf compiler (compared to current OTLP state):

```
BenchmarkEncode/Current/Trace/Attribs-8              	     813	   1479588 ns/op
BenchmarkEncode/Proposed/Trace/Attribs-8             	     417	   2873476 ns/op
BenchmarkEncode/OpenCensus/Trace/Attribs-8           	     162	   7354799 ns/op

BenchmarkDecode/Current/Trace/Attribs-8              	     460	   2646059 ns/op	 1867627 B/op	   36201 allocs/op
BenchmarkDecode/Proposed/Trace/Attribs-8             	     246	   4827671 ns/op	 2171734 B/op	   56209 allocs/op
BenchmarkDecode/OpenCensus/Trace/Attribs-8           	     154	   7560952 ns/op	 2775949 B/op	   76166 allocs/op
```

However, I do not think this is important for most applications. Serialization
CPU and Memory usage is going to be a tiny portion of consumed resources for
most applications, except certain specialized ones.

For the perspective I am also showing OpenCensus in the benchmark to make it
clear that we are still significantly faster than it despite becoming slower
compared to the current state.

More importantly, performance critical applications can use Gogo ProtoBuf
compiler (Collector does use it), which _gains_ performance due to this change:

```
BenchmarkEncode/Current(Gogo)/Trace/Attribs-8        	    1645	    705385 ns/op
BenchmarkEncode/Proposed(Gogo)/Trace/Attribs-8       	    1555	    698771 ns/op

BenchmarkDecode/Current(Gogo)/Trace/Attribs-8        	     537	   2241570 ns/op	 2139634 B/op	   36201 allocs/op
BenchmarkDecode/Proposed(Gogo)/Trace/Attribs-8       	     600	   2053120 ns/op	 1323287 B/op	   46205 allocs/op
```

With Gogo compiler proposed approach uses 40% less memory than the current
schema.

After considering all tradeoffs and alternates (see below) I believe this
proposal is the best overall approach for OTLP. It is idiomatic ProtoBuf, easy
to read and understand, is future-proof to adding new attribute types, has
enough flexibility to represent simple and complex attribute values for all
telemetry types and can be made fast by custom code generation for applications
where it matters using Gogo ProtoBuf compiler.

Note: all performance measurements are done for Go implementation only (although
it is expected that other languages should exhibit somewhat similar behavior).

## Alternates Considered

I also designed and benchmarked several alternate schemas, see below.

### Adding array value to AttributeKeyValue

This is the simplest approach. It doubles down on the current OTLP protocol
approach and simply adds "array_values" field to AttributeKeyValue, e.g.:

```proto
message AttributeKeyValue {
  // all existing fields here.

  // A list of values. "key" field of each element in the list is ignored.
  repeated AttributeKeyValue array_values = 7;
}
```

This eliminates the need to have a separate AnyValue message and has lower CPU
usage because it requires less indirections and less memory allocations per
value. However, this is semantically incorrect since the elements of the array
must actually be values not key-value pairs, which this schema violates. It also
uses more memory than the proposed approach:

```proto
BenchmarkEncode/Proposed/Trace/Attribs-8             	     400	   2869055 ns/op
BenchmarkEncode/MoreFieldsinAKV/Trace/Attribs-8      	     754	   1540978 ns/op

BenchmarkDecode/Proposed/Trace/Attribs-8             	     250	   4790010 ns/op	 2171741 B/op	   56209 allocs/op
BenchmarkDecode/MoreFieldsinAKV/Trace/Attribs-8      	     420	   2806918 ns/op	 2347827 B/op	   36201 allocs/op
```

It will become even worse memory-wise if in the future we need to add more data
types to attributes. This approach is not scalable for future needs and is
semantically wrong.

### Fat AnyValue instead of oneof.

In this approach AnyValue contains all possible field values (similarly to how
AttributeKeyValue is currently):

```proto
message AnyValue {
    ValueType type = 1;
    bool bool_value = 2;
    string string_value = 3;
    int64 int_value = 4;
    double double_value = 5;
    repeated AnyValue list_values = 6;
    repeated AttributeKeyValue kvlist_values = 7;
}
message AttributeKeyValue {
    string key = 1;
    AnyValue value = 2;
}
```

This results in significantly bigger AnyValue in-memory. In vast majority of
cases attribute values of produced telemetry are strings (see e.g. semantic
conventions for proof). Integer and boolean values are also used, although
significantly less frequently than strings. Floating point number, arrays and
maps are likely going to be diminishingly rare in the attributes. If we keep all
these value types in AnyValue we will pay the cost for all these fields although
almost always only string value would be present.

Here are benchmarks comparing proposed schema and schema with fat AnyValue and
using string and integer attributes in spans:

```
BenchmarkEncode/Proposed/Trace/Attribs-8             	     415	   2894513 ns/op	  456866 B/op	   10005 allocs/op
BenchmarkEncode/FatAnyValue/Trace/Attribs-8          	     646	   1885003 ns/op	  385024 B/op	       1 allocs/op

BenchmarkDecode/Proposed/Trace/Attribs-8             	     247	   4872270 ns/op	 2171746 B/op	   56209 allocs/op
BenchmarkDecode/FatAnyValue/Trace/Attribs-8          	     343	   3423494 ns/op	 2988081 B/op	   46205 allocs/op
```

Memory usage with this approach is much higher and it also will become worse as
we add more types.

### AnyValue plus ExoticValue

This is based on fat AnyValue approach but rarely used value types are moved to
a separate ExoticValue message that may be referenced from AnyValue if needed:

```proto
message AnyValue {
    ValueType type = 1;
    bool bool_value = 2;
    string string_value = 3;
    int64 int_value = 4;
    ExoticValue exotic_value = 5;
}
message ExoticValue {
    double double_value = 1;
    repeated AnyValue array_values = 2;
    repeated AttributeKeyValue kvlist_values = 3;
}
message AttributeKeyValue {
    string key = 1;
    AnyValue value = 2;
}
```

While this improves the performance (particularly lowers memory usage for most
frequently used types of attributes) it is awkward and sacrifices too much
readability and usability for small performance gains. Also for the rare cases
it is slow and uses even more memory so its edge case behavior is not desirable.

### Using different schema for log data type

I also considered using a different message definition for LogRecord attributes
and Spans. This would allow to eliminate some of the requirements that we do not
yet formally have for Span attributes (particularly the need to have maps of
nested values).

However, this does not help much in terms of performance, makes Span and
LogRecord attributes non-interchangeable and significantly increases the bloat
of code in applications that need to work with both Spans and Log records.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support array valued attributes Support map values and nested values for attributes
7 participants