Low allocation OTLP marshaler #6328
Conversation
exporters/common/src/main/java/io/opentelemetry/exporter/internal/marshal/MarshalerContext.java
Show resolved
Hide resolved
| @Override | ||
| protected void writeSpanId(ProtoFieldInfo field, String spanId, MarshalerContext context) | ||
| throws IOException { | ||
| byte[] spanIdBytes = idCache.get(spanId); |
There was a problem hiding this comment.
if you add the ability to you ProtoSerializer to writeString then you don't need to fetch a byte array and translate, both for spanId and traceId.
There was a problem hiding this comment.
I don't get what you mean.
| protected abstract void writeTraceId(ProtoFieldInfo field, String traceId) throws IOException; | ||
|
|
||
| protected void writeTraceId(ProtoFieldInfo field, String traceId, MarshalerContext context) |
There was a problem hiding this comment.
Why would the Serializer needs to know about Tracing or any other signal? It's should a protocol, there for agnostic of the business logic of any signal.
There was a problem hiding this comment.
There already are methods for writing trace and span id in the Serializer. I only provided a variant for the ProtoSerializer that uses pooled buffers. Serializer isn't a public api, having special method for trace and span id there is IMO not a big deal.
There was a problem hiding this comment.
Why do you need this? your serializer is works in a single thread right? Just have the writeString(string) have a thread-local ByteBuffer and just re-use it when you need to write the string - convert it to byte array.
There was a problem hiding this comment.
as far as I understand trace id here is a hex string but it isn't marshaled like that
| @@ -22,12 +26,13 @@ | |||
| @ThreadSafe | |||
| public final class OtlpHttpSpanExporter implements SpanExporter { | |||
|
|
|||
| private final HttpExporterBuilder<TraceRequestMarshaler> builder; | |||
| private final HttpExporter<TraceRequestMarshaler> delegate; | |||
| private static final boolean LOW_ALLOCATION_MODE = true; | |||
There was a problem hiding this comment.
This flag is there just to enable the low allocation mode so that tests would run using it. Eventually I'm sure this will be replaced by something else, be it memory mode or some other flag.
...s/otlp/all/src/main/java/io/opentelemetry/exporter/otlp/http/trace/OtlpHttpSpanExporter.java
Show resolved
Hide resolved
...ers/otlp/common/src/main/java/io/opentelemetry/exporter/internal/otlp/KeyValueMarshaler.java
Outdated
Show resolved
Hide resolved
| private static void writeTo( | ||
| Serializer output, MarshalerContext context, AttributeKey<?> attributeKey, Object value) | ||
| throws IOException { | ||
| byte[] keyUtf8 = context.getByteArray(); |
There was a problem hiding this comment.
How can you get a byte array from the pool without passing the size of string you need to encode it into?
There was a problem hiding this comment.
getByteArray isn't for pooling, it reads an element from the data array and casts it to byte[]. This array is produced in the calculateSize method and can add extra allocation if the key is not an instance of InternalAttributeKeyImpl
There was a problem hiding this comment.
I actually don't undetstand why you need to add the objects to the list. I understand adding sizes, but why the objects them selfs?
There was a problem hiding this comment.
It is useful when you don't wish to reconstruct an object that you already constructed when computing the size. For example when you convert a string to utf-8 byte array to get its size this allows reusing the same byte array when writing the data.
|
@laurit Thanks for writing this PR. I'll start by explaining the main idea standing behind this PR, compared my second draft PR: #6273 The design I had in mind when writing it, was that for each Message type, which today as a matching Marshal class for marshalling, I would have 2 methods:
You're basically saying - building a tree of size, means having a pool of How? You suggest representing the size tree as a list. You traverse the data in the I agree that it will have better performance in terms of allocation. To summarize the first big change suggested in this PR: I agree it is much more performant, and can be made readable. I need to try it to see how readable I can make it. Second big change was made to how you serialize repeated element and mostly how the Serializer is not aware of the @jack-berg I can try creating a 3rd draft based on this idea, but it's an effort so I'd like your opinion on the design before implementing it. |
|
I wanted to get familiar with these ideas so I pulled down this branch, and made a little E2E POC of how to use this concept to serialize a simple, contrived proto data structure:
All this is to say that I have a better understanding of what's happening now, and I'm on board with the approach given we do some things to improve readability. |
|
@jack-berg I have modified the PR as you suggested |
|
Sorry for the late reply - had a lecture I was giving at a conference. First, I really love the technique you used here - Take a very big problem, and have a small model, so we can iterate on the concepts of solution/design. Very nice. Second, I like the idea of making the Third, I saw that in your implementation you are only saving the traversed message sizes, and not the data it self - which is an approach I prefer as well. Regarding Where we can choose whether to use Regarding Here we forgot to take into account the field tag size Here we forgot to Regarding and use it : using it looks like: instead of: I'm trying to convey that we have a queue of sizes, and by seeing Enqueue and Dequeue at getBinarySize() and Encode methods, in the same Marshaler class it will make more sense for the reader. It also moves the Queue implementation away from the General question - Once we have an agreement on how to proceed - the design - can I proceed to implementing it in the metrics? Or do you want @jack-berg to do it? How do not step on each others tows, also for traces. |
|
I like where this is heading! Here are steps I see needed to proceed:
@asafm I know your ability to work on this has changed, but I think we work together to land a PR establishing the patterns. After, can work on the other signals in parallel. Can also work on wiring this into the collectors and configuration in parallel after the first signal is landed. @laurit let me know how you want to proceed. Happy to run with any of this or let you adjust scope to land the first PR, depending on your availability. |
I'll start working on it tomorrow. |
|
FYI @laurit - I'm ready to review this whenever you give me the heads up. I still see it has draft status and with a bunch of the metrics changes, which I think we should hold off on to make the review more manageable, so holding off unless until I hear otherwise. |
|
This has been superseded by other PRs. |
This PR aims to explore reducing memory allocations from OTLP marshaling. It takes a slightly different route than #6171 and
#6273 Instead of pooling the
Marshalerobjects here we introduce aMarshalerContextobject that keeps the state needed for marshaling. This context can be reset and reused for subsequent marshaling attempts. Currently low allocation marshaling is implemented only for traces and metrics. My laptop doesn't produce reliable performance numbers, I'd expect the low allocation version to perform slightly worse than the original code.cc @asafm