decompressor filter: add bytes reporting to trailers by junr03 · Pull Request #12477 · envoyproxy/envoy

junr03 · 2020-08-04T22:50:46Z

Commit Message: add bytes reporting to trailers
Additional Description: it is useful for certain experiments to be aware of the compressed/uncompressed size of payloads as they go through the decompressor filter. This PR adds such details to the request/response trailers.
Risk Level: low
Testing: updated unit and integration tests
Docs Changes: updated
Release Notes: updated

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

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

junr03 · 2020-08-04T22:51:50Z

@rojkov do you mind taking a pass at this?

@rebello95 if you want to take a pass too.

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

rojkov · 2020-08-05T05:04:44Z

I'm on holidays till the middle of August. I'll be happy to give it a pass later if it's still open.

rebello95

LGTM overall!

docs/root/configuration/http/http_filters/decompressor_filter.rst

docs/root/version_history/current.rst

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

rebello95 · 2020-08-05T21:11:24Z

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

+      EXPECT_EQ("30",
+                response_trailers
+                    .get(Http::LowerCaseString("x-envoy-decompressor-testlib-compressed-bytes"))
+                    ->value()
+                    .getStringView());
+      EXPECT_EQ("60",


Might be nice to use different body sizes for request/response to also differentiate

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

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

rebello95 · 2020-08-06T00:48:35Z

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

+  decompressor->decompress(input_buffer, output_buffer);
+
+  // Report decompression via stats and logging before modifying the input buffer.
+  byte_tracker.chargeBytes(input_buffer.length(), output_buffer.length());


Checking desired behavior: Do we want to only increment byte counts if we actually do decompression (versus regardless of if we do decompression)?

you mean if decompress had an issue? Or not doing decompression at all?

The latter (i.e., if the response is not compressed)

My opinion is that the trailers should only be added if the stream was actually decompressed. So this is correct. But you did make me realize that the decode/encodeTrailers calls were not gated and that was wrong. Fixed in: 540a5ae

junr03 · 2020-08-06T18:15:00Z

@dio do you mind taking a look given @rojkov is out?

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

dio · 2020-08-11T06:55:37Z

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

+  // TODO: replace std::vector for std::pair. For some reason the macro is failing to build with a
+  // multivariate templated type.


dio

Thank you.

rojkov

This looks great! Just one thing to clarify.

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

stale · 2020-08-24T13:12:22Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-08-31T14:32:59Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

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

junr03 · 2020-09-02T19:39:49Z

coming back to this after being completely occupied otherwise.

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

junr03 · 2020-09-02T22:29:08Z

@dio can I get a re-review now that I updated and de-staled the PR?

dio

Thanks. Looks good!

Bumping primarily to include the changes from envoyproxy/envoy#12477 for decompressor trailers reporting. Contains changes to fix compilation after: - envoyproxy/envoy@d245c02 - envoyproxy/envoy@52ec66f Also contains new fixes required by Envoy Mobile: - envoyproxy/envoy#13039 - envoyproxy/envoy#13040 Signed-off-by: Michael Rebello <me@michaelrebello.com>

Bumping primarily to include the changes from #12477 for decompressor trailers reporting. Contains changes to fix compilation after: - d245c02 - 52ec66f Also contains new fixes required by Envoy Mobile: - #13039 - #13040 Signed-off-by: Michael Rebello <me@michaelrebello.com> Signed-off-by: JP Simard <jp@jpsim.com>

Jose Nino added 9 commits August 3, 2020 16:14

spell

7612d8f

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

more

85d63e8

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

test

2101e24

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

fmt

5f444d0

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

move trailer keys around

89e3f0a

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

tests green

5b0430c

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

trailers test

d799bcc

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

release notes and docs

4daaa11

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

release notes and docs

1d461d0

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

junr03 requested a review from dio as a code owner August 4, 2020 22:50

junr03 requested a review from rojkov August 4, 2020 22:51

junr03 assigned rojkov Aug 4, 2020

clean up assertions

60fe4c4

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

rebello95 previously approved these changes Aug 6, 2020

View reviewed changes

junr03 assigned dio and unassigned rojkov Aug 6, 2020

comments

540a5ae

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

junr03 dismissed rebello95’s stale review via 540a5ae August 7, 2020 17:20

dio reviewed Aug 11, 2020

View reviewed changes

dio previously approved these changes Aug 11, 2020

View reviewed changes

rojkov reviewed Aug 17, 2020

View reviewed changes

source/extensions/filters/http/decompressor/decompressor_filter.h Outdated Show resolved Hide resolved

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 24, 2020

stale bot closed this Aug 31, 2020

Merge branch 'master' into decompressor-trailers

c91d96d

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

junr03 reopened this Sep 2, 2020

junr03 dismissed dio’s stale review via c91d96d September 2, 2020 19:39

stale bot removed stale stalebot believes this issue/PR has not been touched recently labels Sep 2, 2020

Jose Nino added 2 commits September 2, 2020 14:58

fmt after merge

2bf797f

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

rojkov's suggestion

2c04883

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

dio approved these changes Sep 3, 2020

View reviewed changes

junr03 merged commit 02e7f64 into envoyproxy:master Sep 4, 2020

rebello95 mentioned this pull request Sep 9, 2020

upstream: bump envoy submodule ref envoyproxy/envoy-mobile#1076

Merged

		// TODO: replace std::vector for std::pair. For some reason the macro is failing to build with a
		// multivariate templated type.

Conversation

junr03 commented Aug 4, 2020

Uh oh!

junr03 commented Aug 4, 2020

Uh oh!

rojkov commented Aug 5, 2020

Uh oh!

rebello95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rebello95 Aug 5, 2020

Choose a reason for hiding this comment

Uh oh!

junr03 Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rebello95 Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

junr03 Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

rebello95 Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

junr03 Aug 7, 2020

Choose a reason for hiding this comment

Uh oh!

junr03 commented Aug 6, 2020

Uh oh!

dio Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stale bot commented Aug 24, 2020

Uh oh!

stale bot commented Aug 31, 2020

Uh oh!

junr03 commented Sep 2, 2020

Uh oh!

junr03 commented Sep 2, 2020

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants