nit: remove final it does not help here and it just adds noise.

I disagree here - I find that defaulting local variables to final discourages mutating references to objects and results in code that is easier to read and maintain.

final is useful in large methods where there is a higher probability of making a mistake by accidentally reusing the variable. The final should be used to improve the readability of large code blocks or as a "documentation" of the design.
In this PR the final keyword is used for all local variables in very short methods (e.g. 5-10 LOC), which does not make sense to me.

The objective is to write as little code as possible because that is the easiest to read, maintain.... Look at Java std lib/JDK implementation or read the effective java book.

I'm a big fan of effective java! I don't recall an item about final and/or non-final local variables, but I personally find that flipping the switch in the IDE and defaulting to final local variables makes the code easier to maintain and build upon over time. The cost of making variables final is low (because it can be done automatically) and IMO it helps make the code a little more readable because its clear at variable declaration time when a variable is reassigned, rather than relying on the reader to figure out whether a variable is effectively final (because it's never reassigned) or if it's actually mutated. This scales well over time as a function organically grows in length over time and is modified by multiple authors, who may or may not have context on what the previous person wrote. I guess I just disagree that the least amount of code possible is the easiest to read and maintain. A good counterexample to that point, in my opinion, is how books like effective java encourage the use of inheritance over composition. In general, using composition rather than inheritance results in more code (as measured by LOC) but most folks agree that composition is a better default method of sharing code because it is easier to understand what is happening when read in the future. That's not to say that there isn't a place for inheritance (or non-final local variables), just that it's part of what influences what I tend to do by default.

That being said, in the context of this PR it isn't terribly important, so if you feel strongly about this I'd be happy to table the discussion for another time and remove the final keyword from local variables in this PR. We can address it at a time when we can also add in automation to enforce whatever decision we come to.

what size is used if the content length is 0 e.g. for chunked transfer

The BoundedBuffersFactory has some logic to help us out with that. In the case of stream being created in this way, we'll simply use the default ByteArrayOutputStream constructor which uses the default initial size of 32. Regardless, the ByteArrayOutputStream will grow to fit the size of its content as we write to it (until restricted by the BoundedByteArrayOutputStream). I'm sure we can improve on this, but I'm trying to keep this PR scoped to the bug with the customer as I don't have great data on how/when we can improve this right now

We literally do not need this type in the javaagent-core. javaagent-core should contain only types that are used by multiple instrumentations. Which does not seem to be the case here. This type can be moved to undertow module. Also only SERVLET value is used in the codebase.

I think I know your intentions with this type and overall approach, however it won't work as a generic solution across instrumentations. We should design an API that will tell you if payload or headers for a specific request were captured and it should work for both client and server instrumentations (clients have the same problem e.g. jax-rs client implementation is often times backed by apache HTTP client...). The approach in this PR won't work bc it uses instrumentation context with specific types 3rd party library types e.g. io.undertow.server.HttpServerExchange.

You might want to look at https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/3e28b01e42311d755db17d698966d8f3bfaea1e3/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java#L88 to see how to design this using the APIs that are available for all instrumentations.

Definitely agreed, I'll move this type into a common library project shared by both undertow instrumentation projects. Over time, when we have more use-cases like this one, we can design a more general solution. This was definitely premature 🙂

This does not seem efficient, ByteBuffer should have an API to return the allocated array.

ByteBuffer.array() does exist, but it's not guaranteed to work (depending on the kind of ByteBuffer being used. As far as I know, it does not work on DirectByteBuffers, only on heap-based buffers. In the case of undertow, the ByteBuffer subclass being used in the test case is a DirectByteBuffer so ByteBuffer.array() will throw an UnsupportedOperationException.

My first pass at this, after discovering that ByteBuffer.array() does not suit our needs, was to do something like:

byte[] dst = new byte[numBytesRead];
readOnlyBuffer.get(dst);
boundedByteArrayOutputStream.write(dst);

However, after doing this, I looked at the implementation of ByteBuffer.get(byte[], int, int). It looks like this:

public ByteBuffer get(byte[] dst, int offset, int length) {
  checkBounds(offset, length, dst.length);
  if (length > remaining())
    throw new BufferUnderflowException();
  int end = offset + length;
  for (int i = offset; i < end; i++)
      dst[i] = get();
      return this;
}

Note ByteBuffer.get() is called n times, as it is in this method as well. So, by calling ByteBuffer.get() directly here, it is invoked the same number of times as ByteBuffer.get(byte[]), except that we get to avoid allocating a duplicate byte[] just to have it eligible for GC at the end of this utility method. This could put additional GC pressure on the application and result in degraded performance. I'm definitely open to other options of copying data from the ByteBuffer! But as far as I know, this is our best option today.

I believe this is not being used anywhere

I think this should be 1.4.0. It is what muzzle check is configred against. The idea is to use the lowest version possible.

Works for me 👍 FWIW i copied this part from the OTEL undertow module