OTLP

[tigrannajaryan]

No description provided.

[bogdandrutu]

Do we really need an ID? modern protocols have an embedded way to associate requests with response even in an async mode.

[tigrannajaryan]

You are right, ID would not be needed if we used a transport that inherently supports matching of requests and responses (such as unary gRPC). However, as far as I know gRPC stream does not have such functionality. All it supports is ordered stream of messages in both directions. It is up to the application to interpret which response corresponds to which request.

One approach when using streaming would be to rely on the order of the requests and responses. However it places unnecessary limitation on server implementations. I want server implementations to be able to send out-of-order responses to Export requests (think for example a thread-pool based server implementation which gets requests from the stream, processes them in a thread pool and sends responses over the stream. In this situation it is difficult to guarantee the order of responses).

[bogdandrutu]

For the pipelined mode, does it require in case of a service behind an LB that all the requests go to the same instance? Asking to understand if piepline requires biderectional streams or async unary rpcs are enough.

[tigrannajaryan]

The Load Balancing discussion in this spec is targetted for the case when services behind Load Balancer do not require that all the requests go to the same instance. The goal is to allow Load Balancers rebalance traffic when needed.

So, you are right that unary gRPC would be enough from functional perspective, because unary calls can be rebalanced out of the box.

The problem with unary calls is bad performance. Benchmark results show about 1.12 times more CPU usage of unary calls compared to streaming implementation. This is one of the reasons I decided to go with gRPC streams.

[bogdandrutu]

My 2cents here, is that this is a hard problem to solve and I am not sure this is the right way. Let's assume you have 5 collectors, you need to have a global throttler to know when the other service are also overwhelmed or not.

Probably an easier solution to this is to return a FailedRetryable and implement some kind of backoff retry logic on the client. What do you think?

[tigrannajaryan]

My 2cents here, is that this is a hard problem to solve and I am not sure this is the right way. Let's assume you have 5 collectors, you need to have a global throttler to know when the other service are also overwhelmed or not.

You are right that generally it is a hard problem and there are no solutions that apply to all situations.

However in some scenarios the "retry-after" approach can work well. For example when the same set of clients is connected to the same server and send data to the same server, the server can make approximate prediction about how overloaded it is (for example by looking at the length of its incoming queue that it needs to process and the rate of incoming data) and how much it needs the clients to slow down.

If the server is unable to calculate the throttle period, then it can use exponential backoff logic to calculate throttling period or use some fixed number.

This is analogous to Retry-After header in HTTP: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After and how the server can include it when returning status code 429 to ask client to slow down: https://tools.ietf.org/html/rfc6585#section-4

Probably an easier solution to this is to return a FailedRetryable and implement some kind of backoff retry logic on the client. What do you think?

The problem with this approach is that it is not possible for client to understand the reason why the export request failed. I believe it is important for clients to distinguish general retryable errors from the case when the server is overloaded and asks the client to slow down. This is important for observability purposes (to know why exactly the export is failing).

If you think that having the numeric throttle period in the response is superfluous we can get rid of it and only report a boolean "overloaded" status in the response.

[bogdandrutu]

I would like to revisit Node. I think currently because some of the Omnition people pushed me we added some informations like service name which should be part of the Resource.

[tigrannajaryan]

Yes, we need to go over Node/Resource/Span/Metric and decide what will be the schema for this. I left it out from this RFC to limit the scope, I can work on it in a separate RFC and link from this one.

[bogdandrutu]

Should this be implemented on the server? This may be unnecessary overhead if the receiver is otelserv daemon for example. Maybe one of the return property from the server should include a restart stream property?

[tigrannajaryan]

If this functionality is not needed it can be disabled on the client-side (by setting timeout period to infinity).

We can recommend to disable rebalancing in client libraries by default, because they are most likely sending to local daemon.

Agents and Collectors will have this enabled by default.

[bogdandrutu]

One problem with stream which I did not think about is setting a deadline per message. We do want to have a deadline probably for every request correct? I am not sure if this is important or not but please also think about.

Also if we do go with unary RPCs we can remove the ack id because then it is very easy to map request with the response, also the piepline will be implemented using async unary rpc.

I am leaning more towards unary rpcs because you also removed the optimization to not send the node/resource.

[tigrannajaryan]

One problem with stream which I did not think about is setting a deadline per message. We do want to have a deadline probably for every request correct? I am not sure if this is important or not but please also think about.

Yes, client implementations definitely will need some sort of timeout after which they should consider the message to be lost if they don't hear from the server. At that point they have a choice: declare the message dropped (and increment some internal "dropped" counter) or retry sending the message. I think we should go with "dropped" approach, because retrying is risky, we don't know why the server is unable to respond to this message and can end up sending the same message over and over again and potentially overload the server.

I will add this to the spec.

Also if we do go with unary RPCs we can remove the ack id because then it is very easy to map request with the response, also the piepline will be implemented using async unary rpc.

See my other response regarding unary performance and why I think we should use streaming instead.

I am leaning more towards unary rpcs because you also removed the optimization to not send the node/resource.

I did not remove the node/resource optimization. I changes the Export message format to allow to include multiple nodes.

This is OpenCensus export format:

message ExportTraceServiceRequest {
  Node node = 1;
  repeated Span spans = 2;
  Resource resource = 3;
}

This allows one Node, one Resource and multiple Spans.

I suggest to change this to following:

message ExportRequest {
  uint64 id = 1;
  repeated NodeSpans nodeSpans = 2;
  repeated NodeMetrics nodeMetrics = 3;
}
message NodeSpans {
  Node node = 1;
  Resource resource = 2;
  repeated Span spans = 3;
}

NodeSpans looks like ExportTraceServiceRequest in OpenCensus. In OLTP you can have many of these in one ExportRequest (via repeated NodeSpans nodeSpans = 2 field). This is important for Collector use-case.

Collector receives telemetry data from different sources. Different sources means different Node infromation. One of the jobs of a Collector is to combine telemetry data into batches for more efficient processing. However because of how OpenCensus ExportTraceServiceRequest schema is defined it is impossible to batch telemetry data from different sources (different Nodes) into one export request. This can have dramatic performance impact on the efficiency because it can increase the number of Export requests that Collector must do by orders of magnitude.

We observed exactly this problem with OpenCensus protocol just last week.

My proposal fixes this problem, one ExportRequest can contain telemetry data from many Nodes.

[bogdandrutu]

The optimization that you removed is that Node/Resource can be sent once per stream. You did keep the optimization to send Node/Resource once for multiple data

[tigrannajaryan]

I think we can still have the same optimization per stream. It will be a bit more difficult for intermediary nodes like otelsvc to handle this optimization but I think it is doable.

For data recieved from telemetry sources (applications) if Resource is not specified in ExportRequest then the last Resource is applied ("last" means anything that is listed in the same ExportRequest in earlier batch or in the previous ExportRequest). This will allow telemetry sources which always use the same Resource to specify a single SpanBatch in ExportRequest and specify Resource only once, in the first request. Typically ExportRequest will contain only one element in spanBatches array.

otelsvc needs to be a bit more complex to handle this optimization. otelsvc needs to combine SpanBatch-es received from different streams into one Export request (and create bigger batches). In that case ExportRequest will contain an array of SpanBatch-es. If the SpanBatch received from a particular stream does not have a Resource specified than otelsvc will add the Resource field before combining it into multi-batch request. To be able to do so otelsvc receiver must cache the last received Resource per each stream. The otelsvc batcher will also be able to do batch compacting, it can eliminate unnecessary repetitions of the same Resource inside multiple batches received from the same source when creating a multi-batch request, like this (R is Resource, S is Span, rectangular block composed from R and S is one ExportRequest):

[bogdandrutu]

I think we should do it differently. If we do want to do a major change we can simply have an ExportV2 method on the rpc service.

[tigrannajaryan]

I thought about using version numbers in the protocol (what you suggest with ExportV2 is having version numbers, per service call or per protocol). I decided against it for the following reason:

In the future we will likely decide to extend OTLP with various new capabilities. Some of this capabilities will probably be optional, not required for all implementations. So, imagine that we have N optional capabilities and clients and server can implement a subset of this N capabilities. It will potentially we have 2^N versions of the protocol that can exist in the wild which need to be able to talk with each other. It would not be scalable to attempt to label these protocols by versions numbers.

This is the reason I decided to go with the concept of "capabilities" instead of version numbers. When a new capability is added to the protocol, it will be the responsibility of client and server to exchange the capabilities they support during Hello handshake.

[bogdandrutu]

Do we want to support config pushes to the client via the same protocol? Probably not and we should define a different protocol for that.

[tigrannajaryan]

I think it should not be in this RFC. Even if in the future we decide to support config pushes via the same gRPC stream it can be defined in in a future RFC as an extension to this spec.

[bogdandrutu]

Maybe parallel instead of pipelined?

[tigrannajaryan]

I use pipelined as analogy from http pipelining mode: https://en.wikipedia.org/wiki/HTTP_pipelining

I think it describes the mode well. Parallel can be also used but I think analogy with http is useful.

[bogdandrutu]

s/OpenTelemetry Service running/OpenTelemetry Service instance is running

Remove double "as"

[tigrannajaryan]

Fixed.

[bogdandrutu]

Implementing Deadline on top of a bidirectional stream may be a hard and error prone thing. I need to think a bit more about this.

[bogdandrutu]

The optimization that you removed is that Node/Resource can be sent once per stream. You did keep the optimization to send Node/Resource once for multiple data

[bogdandrutu]

Usually metrics have different requirements in terms of retries/load-balancing/reporting periods etc. Having them in the same protocol forces us to apply the same configuration, or at least is less intuitive. Also even in the Receivers we would probably have different configurations for metrics/spans.

Also if any of the OSS backends like Jaeger/Zipkin decide to support this the will support half of the "Service". Having them in different grpc Services makes it more clear that Jaeger supports TraceService (so it supports the entire service).

[bogdandrutu]

I think you want to use a list of pairs not a map. List is faster to be decoded because no map unique key requirements is checked.

[tigrannajaryan]

I thought about this possibility. I agree lists are faster to decode. The problem I see with this is that it will make the in-memory representation of Attributes not very suitable for processing in otelsvc. Processors which need to inspect or modify specific attributes will have to do linear search and linear list modification instead of map lookup by key and it will make processing more expensive. Or otelsvc will need to transform the list into map internally to make processing easier and then transform it back to list before exporting. This will complicate otelsvc implementation.
I am not sure gain in the decoding speed is worth the complexity it brings to otelsvc. I can do some benchmarking to see how much we gain there is.

[tigrannajaryan]

I did benchmarking of Attributes as a map vs list. List is 3 times faster (in this single benchmark):

BenchmarkEncode/OpenCensus-8         	      10	 616829553 ns/op
BenchmarkEncode/OTLP/AttrMap-8       	      10	 529560209 ns/op
BenchmarkEncode/OTLP/AttrList-8      	      50	 144381428 ns/op
BenchmarkDecode/OpenCensus-8         	      10	 640141705 ns/op
BenchmarkDecode/OTLP/AttrMap-8       	      20	 538879696 ns/op
BenchmarkDecode/OTLP/AttrList-8      	      30	 222093182 ns/op

I wasn't expecting so much gains from using list. Definitely worth considering. But we need a good solution for otelsvc (perhaps lazy-initializing the map from the list when needed).

[pjanotti]

If the client is shutting down

This should also apply to the Synchronous case (it is a special case with a single pending ack).

The client implementation SHOULD expose an option to turn on and off the waiting during shutdown.

Since this is prescriptive to implementers I think it is good to say when the option can be changed.

[pjanotti]

It is implicit that this is not the default gRPC server side keepalive settings, perhaps it is worth mentioning that and their interaction with keepalive settings (they have defaults even if not specified). A simplifying assumption to implement these on the server side is that it receives each individual requests, respond to the request, and then checks for RebalanceTimeout and RebalanceRequests thresholds.

[tigrannajaryan]

I opened a PR against oteps repo (open-telemetry#35), let's continue discussion there. Closing this one.