-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Introduce model translation and encoding interfaces #3200
Conversation
This is a somewhat more limited version of open-telemetry#3044. It decouples translation of models from encodings. Models refers to the in-memory representation of a protcol like the zipkin v2 SpanModel. After translating from pdata to the model an encoding is used to serialize the model to a particular byte representation (protobuf, JSON, etc.). The reverse also applies in deserializing an encoding of bytes to a model then translating the model to pdata.
Before the encoder interfaces took pdata and serialized it by calling the translator. This fully separates the concerns by having model do pure translation, encoding do pure serialization, and the transcoder doing both. Without this there was no way to use the encoder if you already had a model. Only updates traces for feedback purposes.
protocols/zipkinv2/encoding.go
Outdated
_ encoding.TracesDecoder = (*Encoder)(nil) | ||
) | ||
|
||
type Encoder struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map[encoding.Type]Encoder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible but will require some helpers. Let's punt on it for now though as it doesn't impact the external API so can be easily tidied up later.
95571ad
to
a87a9ec
Compare
* standardized on encoding/decoding terminology * renamed encodings to bytes to avoid confusion with encode/decode terminology * added high level interfaces to top level protocols package that goes directly pdata <-> bytes
@jrcamp later we may want to add a "compressor" interface for things like https://github.com/open-telemetry/opentelemetry-collector/issues/3223. Or maybe we can use a standard library for that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Top directory should be "model" instead of "protocols", see #3104 for the decision of the package.
Co-authored-by: Tigran Najaryan <[email protected]>
Some feedback based on my quick PR to fix usage of internal protos in fileexporter:
See #3238 |
I looked into the I think both might have their place and maybe the answer is to support both. My concern about only supporting Reader/Writer would be internally the encoder is dealing in bytes, has to return Perhaps both could be supported though? // MetricsMarshaler encodes protocol-specific data model into bytes.
type MetricsMarshaler interface {
MarshalMetrics(model interface{}) (io.Reader, error)
MarshalMetricsBytes(model interface{}) ([]byte, error)
} The protocol would implement one then the default implementation of the unimplemented one would call the implemented one and wrap/unwrap bytes as needed. But that way if you call
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good as a first step. Can you for the moment put it in the internal/model so we don't expose it publicly until we know the exact path we want?
Just return |
) * [wip] Introduce model translation and encodings interfaces This is a somewhat more limited version of open-telemetry#3044. It decouples translation of models from encodings. Models refers to the in-memory representation of a protcol like the zipkin v2 SpanModel. After translating from pdata to the model an encoding is used to serialize the model to a particular byte representation (protobuf, JSON, etc.). The reverse also applies in deserializing an encoding of bytes to a model then translating the model to pdata. * use encode/decode consistently * review feedback * decouple encoding from model Before the encoder interfaces took pdata and serialized it by calling the translator. This fully separates the concerns by having model do pure translation, encoding do pure serialization, and the transcoder doing both. Without this there was no way to use the encoder if you already had a model. Only updates traces for feedback purposes. * add high level interface * standardized on encoding/decoding terminology * renamed encodings to bytes to avoid confusion with encode/decode terminology * added high level interfaces to top level protocols package that goes directly pdata <-> bytes * cleanup * Apply suggestions from code review Co-authored-by: Tigran Najaryan <[email protected]> * renamings * reword error * cleanup * return interface instead of out parameter * review feedback * serialize -> marshal * put in internal * lint Co-authored-by: Tigran Najaryan <[email protected]>
This is a somewhat more limited version of #3044. It decouples
translation of models from encodings.
Models refers to the in-memory representation of a protcol like the zipkin v2
SpanModel. After translating from pdata to the model an encoding is used to
serialize the model to a particular byte representation (protobuf, JSON, etc.).
The reverse also applies in deserializing an encoding of bytes to a model then
translating the model to pdata.
The goal is to be able to have a generic way of translating and encoding to/from pdata from supported types (zipkin, Jaeger, etc.) without having to have explicit knowledge about all those types. For instance kafkareceiver/kafkaexporter can then be made to support any type that adheres to the interfaces without having to have knowledge about any of them specifically.
Open questions
* Currently encoding/decoding are separate interfaces which means you don't have to implement both. Model translation is currently a single interface which means you have to implement both. Should requiring two-way conversion be mandated by the interfaces like in translation or should you be able to implement only one-way conversions like the encode/decode interfaces are?They were made separate interfaces.