[EXPORTER] Optimize OTLP HTTP compression

[chusitoo]

Fixes #2570

Changes

• Added the in-place version of deflate suggested by zlib author.
• Reworked some parts to resolve warnings and make it more C++ like.
• Minor refactoring to the logic in SendRequest to catch and report the error via the callback.
• Removed some redundant temporary string constructors where appropriate.

Another alternative to in-place compression was also considered, which consists in statically allocating the vector that would be used as output to deflate (i.e. compressed_body in the original code) and swapping it with the original body after deflate was done, essentially, reusing the original body vector for the subsequent call to SendRequest.

------------------------------------------------------
Benchmark            Time             CPU   Iterations
------------------------------------------------------
BM_Original      32782 ns        32782 ns        21061
BM_WithSwap      16769 ns        16769 ns        40696
BM_InPlace       16960 ns        16960 ns        42027

The very minimal benchmark showed that the performance was not that far off compared to the in-place version from this PR, but it would obviously fall short in more realistic scenarios, since it will likely require resizing this scratch vector every now and then if the input body being processed is larger.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[netlify]

✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name	Link
🔨 Latest commit	a90f88f
🔍 Latest deploy log	https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/6762f410b5ece700083f5854

[chusitoo]

Here, MAX_MEM_LEVEL appears to be 9 and, from the official documentation

The memLevel parameter specifies how much memory should be allocated for the internal compression state. memLevel=1 uses minimum memory but is slow and reduces compression ratio; memLevel=9 uses maximum memory for optimal speed. The default value is 8.

The default level is not exposed, unfortunately (header zutil.h not available for including), but the code to drive the default is:

#if MAX_MEM_LEVEL >= 8
#  define DEF_MEM_LEVEL 8
#else
#  define DEF_MEM_LEVEL  MAX_MEM_LEVEL
#endif
/* default memLevel */

My intuition was to not hardcode the "default" value 8 and use the max level setting provided by the macro but I can revert this back to a hardcoded value of 8 as in the current compression code.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.82%. Comparing base (2b9bff9) to head (a90f88f).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3178   +/-   ##
=======================================
  Coverage   87.82%   87.82%           
=======================================
  Files         195      195           
  Lines        6154     6154           
=======================================
  Hits         5404     5404           
  Misses        750      750           

[chusitoo]

This is just a precaution; the documentation states that msg is null when there are no errors. Given that we check for an error return code before even attempting to use this variable, the ternary if is probably redundant, but it does not hurt keeping it as another safety net in case the invariant is broken by the custom deflateInPlace code or future versions of the library, for instance.

The list of possible messages to be reported:

z_const char * const z_errmsg[10] = {
    (z_const char *)"need dictionary",     /* Z_NEED_DICT       2  */
    (z_const char *)"stream end",          /* Z_STREAM_END      1  */
    (z_const char *)"",                    /* Z_OK              0  */
    (z_const char *)"file error",          /* Z_ERRNO         (-1) */
    (z_const char *)"stream error",        /* Z_STREAM_ERROR  (-2) */
    (z_const char *)"data error",          /* Z_DATA_ERROR    (-3) */
    (z_const char *)"insufficient memory", /* Z_MEM_ERROR     (-4) */
    (z_const char *)"buffer error",        /* Z_BUF_ERROR     (-5) */
    (z_const char *)"incompatible version",/* Z_VERSION_ERROR (-6) */
    (z_const char *)""
};

[chusitoo]

Here, max == size which means that, if I interpret the comments from the library author correctly, when there is a lot of incompressible data, an auxiliary buffer is used to complete the deflate operation so that it can report the current max value and bail out with an error code. My understanding is that this would require calling deflateInPlace again, this time with an input buffer that has been resized to accommodate for the expected output, which would be larger than the original payload.

[owent]

In my understandarding, compare a signed interger with a unsigned integer will cause a warning. No idea why this warning does not be triggered.

[marcalff]

For ENABLE_OTLP_COMPRESSION_PREVIEW to work, add -DWITH_OTLP_HTTP_COMPRESSION=ON to CMake in the maintainer builds.

[chusitoo]

I've not seen compile warnings related to this but. indeed, the type of have here is ptrdiff_t. At first glance, it does not seem possible that the logic yield a negative number so it could but made unsigned but, given that L96 uses it to advance through the buffer, I think it is more reasonable to keep its intended type and to cast the right hand side of condition in L93 instead.

[marcalff]

LGTM, thanks for the fix.

[marcalff]

Waiting on @owent to clear the change needed flag and approve.

[owent]

Waiting on @owent to clear the change needed flag and approve.

LGTM, but I'm not familiar with zlib APIs.I'm not sure about the details of the usage.

[marcalff]

Blocking merge, to resolve concern with GzipIncompressibleData

[marcalff]

LGTM, thanks for the fix.

Great work.