open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.76k stars 891 forks source link

Support map values and nested values for attributes #376

Closed tigrannajaryan closed 7 months ago

tigrannajaryan commented 4 years ago

After https://github.com/open-telemetry/opentelemetry-specification/pull/368 gets merged we will have support for array values.

If we add support for maps and nesting it will allow to represent arbitrary nested data structures in attribute values if needed.

This will apply to span and resource attributes.

jmacd commented 4 years ago

Can we explicitly state that this applies to resources too? I believe that span attributes and resources (which are only specified in the proto, currently) are specified with the same structure.

Oberon00 commented 4 years ago

Are there any use cases for arbitrary nesting? I think (multi)maps would be useful to store, e.g., HTTP headers, but what would be the rationale for arbitrary nesting?

shengxil commented 4 years ago

Arbitrary nesting map can represent the classified values e.g. {"http" : {"url":...,"method":...}} or {"sql" : {"query":...,"engine":...}}. It can also host vendor specific data like {"aws": {"account_id":...}} in the situations when Resource isn't the right place, e.g. client side metrics

jmacd commented 4 years ago

In #579, Tigran's example seems to contain a use-case. The resource of "application B" is a set of key-value attributes.

mwear commented 4 years ago

Semantically, I agree that the dotted string notation is equivalent to a map, although I'd like to point out, that at least from the tracing client perspective, dotted strings are a slightly more efficient representation.

Consider the following representations for the key-value pair 'http.method': 'GET'.

Dotted string representation { 'http.method': 'GET } To represent this we need 1 map and 2 strings; 3 total objects.

Map representation { { 'http' : { 'method': 'get' } } This requires 2 maps, 3 strings; 5 total objects.

Furthermore, most tracing backends do not support nested attributes (as far as I know), and will need to flatten them into dotted strings. This is something that will either have to been done in the tracing clients during export, or by the backends on ingest.

While I recognize that this does have some advantages in regards to semantics and for the data that can be represented, it does introduce complexity into tracing clients and backends. I'm not saying we shouldn't pursue this proposal, but we should discuss what the actual benefits are, and whether the added complexity is worth the tradeoff.

mwear commented 4 years ago

I'd also like to add that with the dotted-string notation, tracing clients can reduce the runtime string allocations to 0 for attribute keys by introducing constants for semantic conventions (and any other commonly used keys). We would lose this ability by changing to nested maps.

I should also clarify that I am completely ok with array support. It's the nested map support that I have reservations about.

tigrannajaryan commented 4 years ago

@bogdandrutu can you please clarify why is this reopened?

bogdandrutu commented 4 years ago

@tigrannajaryan because of the last week discussion and concerns raised by @mwear

Oberon00 commented 4 years ago

Was closing it even intentional? I can't remember any final decision in this issue. At least it's not documented here?

Oberon00 commented 4 years ago

I guess closing this via a commit into some personal repository (tigrannajaryan/exp-otelproto@1507a2f) was accidental? It's surprising to me that GitHub allows this (I would even be tempted to call that a bug).

tigrannajaryan commented 4 years ago

I guess closing this via a commit into some personal repository (tigrannajaryan/exp-otelproto@1507a2f) was accidental? It's surprising to me that GitHub allows this (I would even be tempted to call that a bug).

Definitely not intentional. I don't understand why does a commit in my personal repo which is not even a fork of this one result in closing an issue here. That's weird. Agree it appears to be a bug.

lizthegrey commented 4 years ago

To clarify: are nested arrays allowed, e.g. [][]int, or only single dimensional arrays e.g. []int

Oberon00 commented 4 years ago

Currently only homogeneous, single-dimensional arrays of primitives are allowed. This issue is about relaxing that. EDIT: Discussion is happening mostly on https://github.com/open-telemetry/opentelemetry-specification/pull/596 recently.

lizthegrey commented 4 years ago

Currently only homogeneous, single-dimensional arrays of primitives are allowed. This issue is about relaxing that.

Great. So my linked PR at the moment meets the spec, but won't once we relax the criteria.

Oberon00 commented 4 years ago

I maintain we should not relax the criteria, but see #596. This has also been discussed in a few SIG Spec meetings.

SergeyKanzhelev commented 4 years ago

Corresponding PR didn't recieve a single approve. https://github.com/open-telemetry/opentelemetry-specification/pull/596#issuecomment-673883652 Meaning nobody is seeking for this feature. Moving this issue to Future milestone.

tigrannajaryan commented 4 years ago

For the record for future discussions: map values and nested values are currently supported at the OTLP protocol level, but are not utilized by the OpenTelemetry API. This issue is to discuss whether we also want to support such values in the API.

bryannaegele commented 4 years ago

Given map values and nested values are supported at the OTLP protocol level, should we treat the current spec of MUST for only primitives and homogeneous lists to be the base requirement and potentially add a MAY for maps and nested values? The expanded types have been working well at a trace level, so strictly implementing the spec would require us to add validations and restrict inputs, which I'm hesitant to add when it's working and we remove the need for added operations.

https://github.com/open-telemetry/opentelemetry-erlang/issues/111

yurishkuro commented 3 years ago

This issue is missing specific use cases / requirements. Here's one: capturing all http headers of a request.

mateuszrzeszutek commented 3 years ago

I have a couple of questions/observations regarding the HTTP headers use case:

  1. If we allow adding nested structures/maps to Attributes, what type should the attribute key represent? Also Attributes? E.g. in Java AttributeKey<Attributes> attributesKey(String key) would be the factory method
  2. How do we serialize nested structures in non-OTLP formats? Currently in Jaeger & Zipkin exporters we're serializing arrays as JSON lists (["a","b","c"]), by analogy should we serialize nested structures as JSON objects? Or should flatten them like describes in this comment https://github.com/open-telemetry/opentelemetry-specification/issues/376#issuecomment-643488960 ?
  3. Now that Attributes are also used in the metrics API, what would happen if you passed a nested Attributes instance to a meter? Since they're not allowed for metrics
bogdandrutu commented 3 years ago

If we allow adding nested structures/maps to Attributes, what type should the attribute key represent? Also Attributes? E.g. in Java AttributeKey attributesKey(String key) would be the factory method

That is a very Java specific problem and the Java SIG should reach consensus.

How do we serialize nested structures in non-OTLP formats? Currently in Jaeger & Zipkin exporters we're serializing arrays as JSON lists (["a","b","c"]), by analogy should we serialize nested structures as JSON objects? Or should flatten them like describes in this comment Support map values and nested values for attributes #376 (comment) ?

JSON is the simplest way. I think we should not enter the business of flattening. Also each protocol can define their ways of doing this, we care mostly about Jaeger and Zipkin.

Now that Attributes are also used in the metrics API, what would happen if you passed a nested Attributes instance to a meter? Since they're not allowed for metrics

They will continue to not be allowed for the moment

Oberon00 commented 3 years ago

If we allow adding nested structures/maps to Attributes, what type should the attribute key represent? Also Attributes? E.g. in Java AttributeKey attributesKey(String key) would be the factory method

That is a very Java specific problem and the Java SIG should reach consensus.

I don't think this is Java-specific at all. This is a problem for all SIGs that want to strongly type their attributes and also for the semantic convention generator. What should the type of the attribute be declared as? A map<string, string>? That's our first complex type (arrays are currently coded by explicitly listing all possible array types string[], double[] etc.). Do we need arbitrary nesting or just maps from atomic primitive to atomic primitive or array thereof?

I think we need to understand that while allowing maps seems conceptually very simple, and also a no-op in OTLP, I think it opens a can of worms regarding tooling.

Also, I know of no backend that supports maps for tracing. @mwear also brought up performance concerns above in https://github.com/open-telemetry/opentelemetry-specification/issues/376#issuecomment-643488960 (and received many upvotes on that comment).

mateuszrzeszutek commented 3 years ago

I don't think this is Java-specific at all. This is a problem for all SIGs that want to strongly type their attributes and also for the semantic convention generator. What should the type of the attribute be declared as? A map<string, string>? That's our first complex type (arrays are currently coded by explicitly listing all possible array types string[], double[] etc.). Do we need arbitrary nesting or just maps from atomic primitive to atomic primitive or array thereof?

Exactly - for the HTTP headers use case the desired type would be map<string, string[]>, but what about other uses? Should we have to add map<string, string> too? Or map<string, ?>, map<int, string> etc. Or just say that we support nesting Attributes (which is sort of a map) inside Attributes.

I think we need to understand that while allowing maps seems conceptually very simple, and also a no-op in OTLP, I think it opens a can of worms regarding tooling.

Also, I know of no backend that supports maps for tracing. @mwear also brought up performance concerns above in #376 (comment) (and received many upvotes on that comment).

So the question here is: is it really worthwhile to introduce map-valued/nested attributes? Do we have any other use case than HTTP attributes? Because if it's only that one use case the cost of implementing that seems to be pretty high (new attribute key type support + exporter changes in all language SIGs, collector included; backend support).

tigrannajaryan commented 3 years ago

FYI, OTLP and Collector define the attributes field as map<string,AnyValue>, where AnyValue is one of the supported value types, including a map, so it is a recursive definition. (Technically the map is a represented as key/value list in OTLP wire protocol, but the semantics are of a map).

Because if it's only that one use case the cost of implementing that seems to be pretty high (new attribute key type support + exporter changes in all language SIGs, collector included; backend support).

No changes are needed in the Collector, it is already fully supported.

Oberon00 commented 3 years ago

No changes are needed in the Collector, it is already fully supported.

How does the collector handle exporting a nested map to Jaeger / Zipkin? Does it already JSON-encode?

tigrannajaryan commented 3 years ago

Also, I know of no backend that supports maps for tracing.

From what I understand AS X-Ray does: https://docs.aws.amazon.com/xray/latest/devguide/xray-concepts.html#xray-concepts-annotations

From the above link:

Metadata are key-value pairs with values of any type, including objects and lists

tigrannajaryan commented 3 years ago

No changes are needed in the Collector, it is already fully supported.

How does the collector handle exporting a nested map to Jaeger / Zipkin? Does it already JSON-encode?

Yes. Jaeger translation calls AsString() which converts maps and arrays to JSON string.

I didn't check Zipkin, it should do the same.

mateuszrzeszutek commented 3 years ago

Also, I know of no backend that supports maps for tracing.

From what I understand AS X-Ray does: https://docs.aws.amazon.com/xray/latest/devguide/xray-concepts.html#xray-concepts-annotations

From the above link:

Metadata are key-value pairs with values of any type, including objects and lists

It's worth noting that AWS Metadata are not indexed/searchable:

Metadata are key-value pairs with values of any type, including objects and lists, but that are not indexed. Use metadata to record data you want to store in the trace but don't need to use for searching traces.

If you don't want to search by specific metadata values and just treat the whole object as one, indivisible entity then you might as well just serialize them to JSON and put them all into a string attribute; there's no real need to have a nested structure of attributes for them.

tigrannajaryan commented 3 years ago

I can see a few exporters (in addition to X-ray) in the Collector which treat the nested like maps instead of flattening or converting to JSON or strings, so supposedly their corresponding backends support nested structures. I think this issue requires more feedback from vendors before we decide what to do about it.

scheler commented 2 years ago

So the question here is: is it really worthwhile to introduce map-valued/nested attributes? Do we have any other use case than HTTP attributes? Because if it's only that one use case the cost of implementing that seems to be pretty high (new attribute key type support + exporter changes in all language SIGs, collector included; backend support).

Is there an alternative suggested to represent map data in attributes? One of the instrumentation libraries in JS sdk for browser sends map data as Span Events which is very inefficient.

{
    "traceId": "bb2a647c845bb6d68dc415042f0ee49d",
    "name": "documentLoad",
    "id": "69a3580422564f5c",
    "kind": 0,
    "timestamp": 1654107849066400,
    "duration": 121600,
    "attributes": {
        "component": "document-load",
        "http.url": "http://localhost:5002/",
        "http.user_agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/102.0.5005.61 Safari/537.36",

    },
    "status": {
        "code": 0
    },
    "events": [

      {

}

        {
            "name": "fetchStart",
            "attributes": {},
            "time": [
                1654107849,
                66400100.00000001
            ]
        },
        {
            "name": "unloadEventStart",
            "attributes": {},
            "time": [
                1654107849,
                94600100
            ]
        },
        {
            "name": "unloadEventEnd",
            "attributes": {},
            "time": [
                1654107849,
                97000100
            ]
        },
        {
            "name": "domInteractive",
            "attributes": {},
            "time": [
                1654107849,
                104000100
            ]
        },
        {
            "name": "domContentLoadedEventStart",
            "attributes": {},
            "time": [
                1654107849,
                187800100
            ]
        },
        {
            "name": "domContentLoadedEventEnd",
            "attributes": {},
            "time": [
                1654107849,
                187900100
            ]
        },
        {
            "name": "domComplete",
            "attributes": {},
            "time": [
                1654107849,
                188000100
            ]
        },
        {
            "name": "loadEventStart",
            "attributes": {},
            "time": [
                1654107849,
                188000100
            ]
        },
        {
            "name": "loadEventEnd",
            "attributes": {},
            "time": [
                1654107849,
                188000100
            ]
        },
        {
            "name": "firstPaint",
            "attributes": {},
            "time": [
                1654107849,
                106000100
            ]
        },
        {
            "name": "firstContentfulPaint",
            "attributes": {},
            "time": [
                1654107849,
                106000100
            ]
        }
    ]
}
Oberon00 commented 2 years ago

The alternative is to use dot notation (or some other convention) in your key names so instead of nesting foo under bar, you use bar.foo as key. This is actually used in the semantic conventions for HTTP headers: https://github.com/open-telemetry/opentelemetry-specification/blob/6522c2f71708cef3e4914aeba609035634529cf0/specification/trace/semantic_conventions/http.md#http-request-and-response-headers

If your map values are all of the same type and you only need one nesting level, you can also use two parallel arrays, one for the keys and one for the values, such that keys[i] is the key for values[i].

I don't see any nested attributes in the example JSON you posted.

scheler commented 2 years ago

This is actually used in the semantic conventions for HTTP headers

This is not generalized though - there is no clarity on when the receivers should convert the dotted keys to maps. A reserved keyword, say, _map_ would have been better. so when you see http.request.headers._map_.<key>, you know http.request.headers is a map. Is it possible to have reserved keywords in attribute name components?

For the JSON I posted, it would look like this if there was support for nested attributes:

    "attributes": {
        "timingData": { 
           "fetchStart":  [
                1654107849,
                66400100.00000001
            ],
           "unloadEventStart": [
                1654107849,
                94600100
            ],
            "unloadEventEnd": [
                1654107849,
                97000100
            ]
        }
   }
Oberon00 commented 2 years ago

there is no clarity on when the receivers should convert the dotted keys to maps.

What would be the use case for converting to a map on the receiver? If you need a map for a particular use case, then you know that particular data, e.g. in a particular namespace, is to be interpreted as a map. If you don't know how to interpret the data, what benefit would you have from a map?

For the JSON I posted, it would look like this if there was support for nested attributes

This looks like you you want to have events instead of nested attributes, since events are meant exactly for the case of exactly marking points in time, and backends may have nice visualizations for them. For "unload" you may also consider a (child) span, since it has start & end.

scheler commented 2 years ago

What would be the use case for converting to a map on the receiver?

Having a 2-way conversion provides way for future interoperability. It would be helpful if the flattened data needs to be converted back to maps.

I support map type for attribute values across resources, spans and logs, flattening them during export wherever required using a certain reversible convention.

Regarding @mwear 's argument on additional string allocations with maps, note that it does not apply to the 2 use-cases described here so far - for http.request.header.<key> and for the timingData example above, the keys within the map are static. Although the concern is valid in general, I think there's a bigger benefit in having a smaller payload on the wire.

Oberon00 commented 2 years ago

If you want smaller payload, you should first enable transport compression, then minimize the number of spans you create, the number of attributes you set on the span. I don't think this is a good enough argument for map values.

scheler commented 2 years ago

Smaller payload is a side benefit. The primary motivation for this ask is that the data is generated as a map, and the receivers need them as maps. So, the ask for map support for attribute values is to avoid unnecessary flattening during transport.

tsloughter commented 2 years ago

What is the status of this? Is it just in need of someone to write the spec PR?

tigrannajaryan commented 2 years ago

What is the status of this? Is it just in need of someone to write the spec PR?

Not everyone in the community believes it is necessary (see above). So, it is not just to write a PR, but also to find arguments that will convince people.

tsloughter commented 2 years ago

Ah ok, I guess I need to take a closer look.

The main "objection" I saw seemed to be more of an alternative implementation and not an objection to map values in the API, but in the protocol -- i.e. the dot-notation based approach, which could be a translation of the user level map used in the API to the proto representation.

cartermp commented 2 years ago

(this was also posted in slack)

Dunno if this helps regarding motivation, but:

In my work, we're getting customers who are attaching objects as json-encoded strings as attributes on spans. They would like our backend to "splat" that object into fields in the backend so that they can query for the presence of a value in one of the fields in that object. However, without a way to know for sure that the string is actually a JSON-encoded object, telemetry backends need to scan every string in every attribute and run JSON validation, which is extremely expensive. The same would apply to any span processor in the collector. These customers are not necessarily looking to use logs to solve this problem. It would be seen by them as "going back" to have to use logs to capture this data since they're already using spans to capture app-specific information.

The current thing we're telling customers to do is to see if they can live without it. That doesn't appear to work. Another is to ask them to manually encode an object into each span. That's really difficult for larger objects, or a lot of smaller objects. We could consider a collector processor that allows them to configure keys that they know up-front represent objects, and then unpack only those (or do the same in a telemetry backend). But that's also not ideal, because developers in a large organization may not have access to the collector config they're using, and it's just additional work for them to have to coordinate collector config changes with instrumentation they write in the app (or backend changes in a tool). Another option is to switch your language to JavaScript, since addAttributes takes advantage of the fact that JS objects are key-value pairs that can be represented as a set of attributes (to an extent). But clearly, asking someone to use a different language isn't a good solution.

In the interim, having an endorsed way around this problem that people can follow (and not just my customers) would be great to know so that we can document it in the docs.

tsloughter commented 2 years ago

To build on what @cartermp is saying, I also think the user's need for querying is why it should be typed and not just string values in the protos. Maybe no system has this ability today but knowing the type is an integer of some attribute within a map means both the backend can more efficiently store and search that value but the user can be provided more ways of querying the data (without having to wrap their query reference to the attribute value in atoi).

tigrannajaryan commented 2 years ago

In my work, we're getting customers who are attaching objects as json-encoded strings as attributes on spans. They would like our backend to "splat" that object into fields in the backend so that they can query for the presence of a value in one of the fields in that object.

One alternate way that this particular use case can be addressed is by allowing to record the fact that a string is a JSON in the attribute value. This would allow the backends to know when to unparse it as a JSON. There are downsides to this though since we require everyone who wants to read/modify any attribute (e.g. the Otel Collector) to have special processing logic that deserializes JSON, reads/modifies, then serializes again. It may result in awkward user experience where operations on values inside JSON-encoded fields have their own syntax to use in the Otel Collector processors. I am not sure I like this approach, but wanted to list it for completeness. IMO, first-class support for non-primitive types is a more flexible and powerful alternate.

Oberon00 commented 2 years ago

The main "objection" I saw seemed to be more of an alternative implementation and not an objection to map values in the API,

My main objection is that it makes the whole data model more complicated, from the APIs to the backend processing, as well as the semantic conventions (should I use . or a subobject?) and the semantic convention tooling (what syntax do we use for sub-objects, since the dot is already used in attribute names?)

tsloughter commented 2 years ago

Ok, so I misunderstood and it is a full on objection to having nested values in attributes?

Are there cases where use of . in the semantic conventions would change the meaning if it was considered nesting instead of a single name? My thinking was all those .'s in conventions already are basically representing nesting, so there would be no conflict.

Oberon00 commented 2 years ago

My thinking was all those .'s in conventions already are basically representing nesting, so there would be no conflict.

So are you suggesting that after supporting nested attributes, every every semantic convention (that uses a dot, which is every single one) will be sent as a nested attribute? What about "service.name" which was declared stable already?

tsloughter commented 2 years ago

I'm suggesting they would be considered to be nested. service.name and service.instance.id are attributes under service. And upgrade to the protocol would introduce actual maps but would have to support the . form as well. OTLP servers would have to support both to not require a major version bump and continue to support the stable form service.name.

I don't work on the collector so am only voicing a possibility, the work certainly could be way more than the potential advantages -- many deeply nested attributes when keeping the dot notation as the only option would be duplicating the strings a lot.

But just stating the dot notation represents nested would allow backends to know they can split up these attributes and still get typed values for each nested attribute when they index and search. Just means accepting the memory duplication you wouldn't have with shoving in a json blob.

tsloughter commented 2 years ago

True, I was mainly thinking about the client not wanting to have a major bump, but you are right, it would be a major bump, so I guess off the table.

In which case only supporting the . notation as nesting or adding maps as values would work that don't involve telling users to shove json blobs in -- which I'm not a fan of because it means a user has to do data -> json -> pb and include a json library.

tigrannajaryan commented 2 years ago

Going from dot-notation of attribute names to hierarchical is a different topic. If you would like to discuss it please open a separate issue to avoid confusion.

This issue is about allowing attribute values which are maps or other nested values. For example I may want to record the following information about a TCP connection:

{
  "source.endpoint": { "host": "client.com", "port": 1234 }
  "destination.endpoint": { "host": "server.com", "port": 2345 }
}

Note that each attribute's value is a map.

Oberon00 commented 2 years ago

I guess this discussion branched from my above comment:

the semantic convention tooling (what syntax do we use for sub-objects, since the dot is already used in attribute names?)

I.e. how would you define a semantic convention that you may have an object with host and port properties under an attribute name source.endpoint (probably you would have to significantly enhance the semantic convention generator to support complex types, especially if the intent is to an endpoint object of the same shape in different locations without redefining it each time), and how would you denote the full "path" of the host property here (maybe with source.endpoint#host, borrowing from Javadoc), or are we sure we'd never want that (probably we do want though)?

tsloughter commented 2 years ago

Right, I think it is the same topic (as in, instead of adding maps, declare source.endpoint.host and source.endpoint.port can/should be parsed into maps like your example by a backend, bc otherwise I saw what looked like a deadend, hope I'm wrong and maps get accepted). But I'll will cede to @tigrannajaryan saying it is and not continue that discussion on this thread :)