decompressor library: add stats to zlib library by junr03 · Pull Request #11782 · envoyproxy/envoy

junr03 · 2020-06-27T00:20:37Z

Commit Message: adding decompressor error stats to the zlib decompressor library.
Additional Description: lots of boilerplate, but will make it easier to continue to add stats in the future. Additionally, the compressor library can use the same pattern.
Risk Level: low
Testing: updated tests
Docs Changes: added documentation.

Signed-off-by: Jose Nino jnino@lyft.com

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-06-27T00:21:32Z

@rojkov we saw some decompression errors, and felt that it warranted adding stats here. Let me know what you think!

Signed-off-by: Jose Nino <jnino@lyft.com>

rojkov

Thank you! That's a good addition. I might need a similar patch for the compressor part.

rojkov · 2020-06-29T11:49:33Z

source/extensions/filters/http/decompressor/decompressor_filter.h


  Compression::Decompressor::DecompressorPtr makeDecompressor() {
-    return decompressor_factory_->createDecompressor();
+    return decompressor_factory_->createDecompressor(stats_prefix_ + "decompressor_library");


Can we make this stat prefix be a part of decompressor_factory_'s state? So that this string concatenation would happen only once at Envoy's start rather than for every stream creation.

I debated this back and forth because I am not sure if we would have a place where we would want each decompressor created by the same factory to have different strings.

lets just do it the way you suggest. Having the string concatenation in the hot path seems like a bad price to pay for the niche ability to configure stats trees differently for individual decompressors.

nvm all of the above. I now do understand what you mean. Let me know if what I have pushed is not it.

That's definitely better. Though I was thinking about changing the meaning of DecompressorFactory::statsPrefix(). Instead of returning .gzip it could return the full root stats prefix since we create a factory per DecompressionFilterConfig anyway.

Then DecompressorFilterConfig::stats_prefix_ wouldn't be needed and DecompressorFactory::createDecompressor() would retain its current signature.

I didn’t want to do that because I don’t want the filter stats to have the decompressor_library prefix. To me the public statsPrefix function is for a consumer to use. And the argument in createDeconoressor is what the consumer wants to ultimately root the decompressor at.

Alright, fair enough.

rojkov · 2020-06-29T11:58:22Z

source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.cc

+              "zlib decompression error: {}, msg: {}. Error codes are defined in "
+              "https://www.zlib.net/manual.html",
+              result, zstream_ptr_->msg);
+    stats_.decompression_error_.inc();


Is it enough for your use case not to differentiate errors by their type?

I'm thinking that it might be useful to check the stats and see what's wrong immediately (e.g. is it Z_DATA_ERROR or Z_MEM_ERROR?) instead of increasing log verbosity to trace.

The issue is that even a Z_DATA_ERROR can have dozens of different reasons for occurring, so you would still need to look at logs to get at the heart of it. However, I do think your idea of differentiating based on returned error code is good. I'll make the change.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-06-29T23:02:42Z

@rojkov updated! I am going to be taking time off the rest of this week, but @rebello95 will take a look at this PR after you give it another look and proceed accordingly.

Signed-off-by: Jose Nino <jnino@lyft.com>

This reverts commit bc382b0. Signed-off-by: Jose Nino <jnino@lyft.com>

Signed-off-by: Jose Nino <jnino@lyft.com>

rojkov

Looks good! Just a couple of questions.

rojkov · 2020-07-01T07:37:57Z

test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc

 }

+/**
+ * Exercises gzip decompression bidirectionally with default configuration.


I'd replace "default configuration" with something like "configuration using wrong value for Zlib's windowBits".

bad copy paste, sorry!

rojkov · 2020-07-01T07:41:46Z

test/extensions/filters/http/decompressor/decompressor_filter_integration_test.cc

+  ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_));
+
+  EXPECT_TRUE(upstream_request_->complete());
+  TestUtility::headerMapEqualIgnoreOrder(


Shouldn't we set expectations for these equality checks? Looks like the return value of the check is simply ignored.

yeah, they were bad before. Thanks for noticing, updated!

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-07-07T00:38:23Z

@rojkov updated!

rojkov

Probably one comment line needs polishing, but the rest looks good to me.

rojkov · 2020-07-07T11:12:00Z

source/extensions/compression/gzip/decompressor/zlib_decompressor_impl.h

  int decompression_error_{0};

 private:
+  // TODO: clean up friend class. This is here to allow coverage of chargeErrorStats as it isn't


Nit: you may want to have TODO(junr03) in this line.

I like leaving todos I don't intend to do right away without a username that way it doesn't dissuade others to fix todos they find.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2020-07-07T19:38:42Z

@dio this is ready for a final pass, do you mind taking it?

dio

Thank you! This is good! Sorry for the late reply!

dio · 2020-07-10T17:24:10Z

test/extensions/filters/http/decompressor/decompressor_filter_test.cc

      {"content-length", "256"}};
  std::unique_ptr<Http::RequestOrResponseHeaderMap> headers_after_filter =
      doHeaders(headers_before_filter, false /* end_stream */);
-  TestUtility::headerMapEqualIgnoreOrder(headers_before_filter, *headers_after_filter);


Just curious. So here, we don't expect headers_before_filter to be the same as *headers_after_filter since there is no EXPECT_THAT() replacement here.

Signed-off-by: Jose Nino <jnino@lyft.com>

dio

Thank you!

Description: Pulls in multiple fixes committed to upstream Envoy. - Update for resolution to TLSContext crash: envoyproxy/envoy#10030 - Fixes for 32 bit archs: envoyproxy/envoy#11726 - Fix for missing posix call on Android: envoyproxy/envoy#12081 - Additional zlib stats: envoyproxy/envoy#11782 Signed-off-by: Mike Schore <mike.schore@gmail.com>

This adds decompressor error stats to the zlib decompressor library. This introduces a lot of boilerplate but will make it easier to continue to add stats in the future. Additionally, the compressor library can use the same pattern. Risk Level: Low. Testing: Updated. Docs Changes: Added. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

Description: Pulls in multiple fixes committed to upstream Envoy. - Update for resolution to TLSContext crash: #10030 - Fixes for 32 bit archs: #11726 - Fix for missing posix call on Android: #12081 - Additional zlib stats: #11782 Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

Jose Nino added 7 commits June 24, 2020 10:59

wip

5b4033e

Signed-off-by: Jose Nino <jnino@lyft.com>

wired up

15baceb

Signed-off-by: Jose Nino <jnino@lyft.com>

green

98c7fba

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

bd36392

Signed-off-by: Jose Nino <jnino@lyft.com>

green

fbf4bc2

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'master' into decompressor-stats

e74988f

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

62ff4b8

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 requested a review from dio as a code owner June 27, 2020 00:20

junr03 requested a review from rojkov June 27, 2020 00:20

junr03 assigned rojkov Jun 27, 2020

junr03 changed the title ~~Decompressor stats~~ decompressor library: add stats to zlib library Jun 27, 2020

fix

4d26676

Signed-off-by: Jose Nino <jnino@lyft.com>

rojkov reviewed Jun 29, 2020

View reviewed changes

Jose Nino added 2 commits June 29, 2020 10:56

clang-tidy

da5338a

Signed-off-by: Jose Nino <jnino@lyft.com>

comments

3558b96

Signed-off-by: Jose Nino <jnino@lyft.com>

Jose Nino added 7 commits June 29, 2020 16:41

test ci

b9f7925

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

3461fca

Signed-off-by: Jose Nino <jnino@lyft.com>

add back

02c34fd

Signed-off-by: Jose Nino <jnino@lyft.com>

why is this test hanging in CI but not locally?

bc382b0

Signed-off-by: Jose Nino <jnino@lyft.com>

Revert "why is this test hanging in CI but not locally?"

6c07141

This reverts commit bc382b0. Signed-off-by: Jose Nino <jnino@lyft.com>

facepalm

cf0f748

Signed-off-by: Jose Nino <jnino@lyft.com>

crud

4ce4063

Signed-off-by: Jose Nino <jnino@lyft.com>

rojkov reviewed Jul 1, 2020

View reviewed changes

Jose Nino added 2 commits July 6, 2020 12:03

Merge branch 'master' into decompressor-stats

6e9e0a3

Signed-off-by: Jose Nino <jnino@lyft.com>

comments

b7e7458

Signed-off-by: Jose Nino <jnino@lyft.com>

rojkov reviewed Jul 7, 2020

View reviewed changes

make less flaky

d2b753f

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 assigned dio Jul 7, 2020

dio previously approved these changes Jul 10, 2020

View reviewed changes

Jose Nino added 2 commits July 11, 2020 16:14

comment

469e75f

Signed-off-by: Jose Nino <jnino@lyft.com>

comment

3e7be62

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 dismissed dio’s stale review via 3e7be62 July 11, 2020 23:16

Merge branch 'master' into decompressor-stats

e06a96e

Signed-off-by: Jose Nino <jnino@lyft.com>

dio approved these changes Jul 12, 2020

View reviewed changes

dio merged commit 84affaa into envoyproxy:master Jul 12, 2020

rebello95 mentioned this pull request Jul 14, 2020

upstream: update Envoy upstream sha envoyproxy/envoy-mobile#951

Closed

goaway mentioned this pull request Jul 15, 2020

envoy: update upstream for multiple fixes envoyproxy/envoy-mobile#923

Merged

Conversation

junr03 commented Jun 27, 2020 • edited by dio Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

junr03 commented Jun 27, 2020

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Jun 29, 2020

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Jul 7, 2020

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Jul 7, 2020

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

junr03 commented Jun 27, 2020 •

edited by dio

Loading