Add proto messages for signals data independent of OTLP protocol#332
Conversation
|
@tigrannajaryan PTAL |
|
Please add a CHANGELOG entry for the release notes. |
| // | ||
| // When new fields are added into this message, ExportTraceServiceRequest MUST be updated | ||
| // as well. | ||
| message Traces { |
There was a problem hiding this comment.
Is it ok that a structure Traces will generally not contain full traces, just the part that is local to a component? It seems like it could be confusing (ExportTraceRequest doesn't have this problem since it's explicitly about exporting).
I'd expect a structure Traces to be grouped by trace, not resource. An official proto for such would be very helpful, but in the meantime ExportedResourceSpans or just ResourceSpansList seems more clear.
Other signals coincidentally don't have this issue since they're not distributed, but traces are unique here I'd say
There was a problem hiding this comment.
Indeed it may be a bit confusing, but on the other side I really like the consistency.
Maybe rename everything with a suffix 'Data'?
There was a problem hiding this comment.
Data came to mind to and might be OK. Though the List suffix seems dead simple and pretty clear too
There was a problem hiding this comment.
Yeah, List or Collection or similar should work.
There was a problem hiding this comment.
ResourceSpansList or TracesList? Not sure I understand the proposal exactly
There was a problem hiding this comment.
TracesList sounds like it is full traces like @anuraaga said. ResourceSpansList seems safer.
There was a problem hiding this comment.
ResourceSpansList is bad because the whole idea is that we may possibly add extra things. Naming very specific like that is not my top preference
There was a problem hiding this comment.
Picked "TracesData" since again see my previous comment, one of the reason to have this as independent message is to ensure that people embedding this message will have all the "data" fields that a signal may have, excluding protocol specific metadata.
|
I am not sure what the motivation for this is. Could somebody expand on why this is important? |
Why are the existing messages not suitable for that? For example, why do we need What is more, we seem to have only a very vague idea of who would be using this message type. I think chances are high that whoever would need these messages needs to make some tiny adjustments anyway to them and can't use them as-is. Since the messages themselves are trivial to implement, I don't see the value in providing them here. |
|
@Oberon00 that is kind of missunderstanding of the motivation, or me missunderstanding your suggestion. I explained also in the meeting, currently we recommend users (including the collector) to serialize the Request object on the disk or in Kafka, but this seems wrong if the request will include protocol specific metadata. Also others are not willing to embed our Request object in their protocols (name definitely does not help) see Jaeger example in their query API, these new messages will be guaranteed to contain only the data and nothing else more. Users can still include extra things as siblings to the data. Another reason is that if we add something else to data and users will include only the repeated list they will have a hard time upgrading (not going to happen automatically). So it is critical for us to separate Data from protocol specific metadata in our protos, and as tried to explained in the proto comments it is very hard right now (backwards incompatible) to include this in the Request objects that we have, that would have been the cleanest way. |
The main motivation for this is to ensure that other protocols can use it to send/receive OTLP data, as well as for persistent storages such as kafka, disk, etc. Signed-off-by: Bogdan Drutu <[email protected]>
jsuereth
left a comment
jsuereth
left a comment
There was a problem hiding this comment.
Agree with name change.
|
Change name looks fine! |
10 participants
The main motivation for this is to ensure that other protocols can use it to
send/receive OTLP data, as well as for persistent storages such as kafka, disk, etc.
Unfortunately we cannot use the same message in the OTLP request messages since it will be a breaking change,
but we can ensure that all fields present in the data message will be added to the request message as well.
Signed-off-by: Bogdan Drutu [email protected]