tech debt: simplify access log code by adding a ALL_RESPONSE_STRING_FLAGS by lambdai · Pull Request #15580 · envoyproxy/envoy

lambdai · 2021-03-19T21:22:05Z

Commit Message:
Remove a bunch of duplicated code in the access log. Adding new response flag will be easier.

Signed-off-by: Yuchen Dai silentdai@gmail.com

Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-03-20T01:54:03Z

https://storage.googleapis.com/envoy-pr/88caf8e/coverage/source/common/stream_info/index.html

The coverage is lower because I rewrite the verbose code 😭

source/common/stream_info/utility.cc

jmarantz · 2021-03-24T12:39:52Z

/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

source/common/stream_info/utility.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz

looks great; just a few small comments. Thank you!

source/common/stream_info/utility.cc

source/common/stream_info/utility.h

test/per_file_coverage.sh

jmarantz · 2021-03-31T04:02:23Z

/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz · 2021-04-04T23:57:52Z

/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz

just a minor perf nit

source/common/stream_info/utility.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

source/common/stream_info/utility.cc

jmarantz

Great, thanks!

Might need to merge main to try to pass CI?

@envoyproxy/senior-maintainers

alyssawilk · 2021-04-08T12:31:58Z

CI is broken right now but
/assign @envoyproxy/senior-maintainers

repokitteh-read-only · 2021-04-08T12:32:01Z

@envoyproxy/senior-maintainers cannot be assigned to this issue.

🐱

Caused by: a #15580 (comment) was created by @alyssawilk.

see: more, trace.

alyssawilk · 2021-04-08T12:32:33Z

boo. @htuch @itayd did I do the assign incorrectly or is it opt-in per group?

edit: right, assign-from not assign. though that doesn't seem to work either. Thoughts?

alyssawilk · 2021-04-08T12:40:30Z

/assign-from @envoyproxy/senior-maintainers

alyssawilk · 2021-04-08T12:42:11Z

hm, I think I have the right command this time and it's still not working. #15594 is merged - do we need to update anything else to get it to work?

htuch · 2021-04-08T14:03:23Z

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2021-04-08T14:03:26Z

@envoyproxy/senior-maintainers assignee is @mattklein123

🐱

Caused by: a #15580 (comment) was created by @htuch.

see: more, trace.

htuch · 2021-04-08T14:04:32Z

@itayd any idea it didn't pick up when @alyssawilk tried? It looks like no errors on #repokitteh and no indication of RK observing the comment. @alyssawilk can you try out the command in some other PRs?

mattklein123

Thanks nice cleanup! A few small comments.

/wait

source/common/stream_info/utility.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

itayd · 2021-04-09T17:33:25Z

@htuch seems like a transient github error? from the trace it returned 404, but after that was ok.

github_call(
  method="GET",
  path="repos/envoyproxy/envoy/assignees/envoyproxy/senior-maintainers",
  body=null,
  success_codes=[204,404],
  accept=null,
) -> (
  {"body":"","json":null,"links":{},"status":"404 Not Found","status_code":404}, (
  null, 
)

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2021-04-09T20:13:13Z

Do I need to merge main?

mattklein123

Thanks LGTM with small comment.

/wait

source/common/stream_info/utility.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123

Thanks!

…LAGS (envoyproxy#15580) Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai added 2 commits March 19, 2021 01:07

access log: simplify code by adding ALL_RESPONSE_STRING_FLAGS

88caf8e

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

update per file coverage

ba72cd8

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz reviewed Mar 21, 2021

View reviewed changes

source/common/stream_info/utility.cc Outdated Show resolved Hide resolved

jmarantz self-assigned this Mar 21, 2021

repokitteh-read-only bot added the waiting label Mar 24, 2021

string_view flag

0dde904

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Mar 26, 2021

jmarantz reviewed Mar 26, 2021

View reviewed changes

source/common/stream_info/utility.cc Outdated Show resolved Hide resolved

remove useless comment

7cddc8b

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz reviewed Mar 27, 2021

View reviewed changes

source/common/stream_info/utility.cc Outdated Show resolved Hide resolved

source/common/stream_info/utility.h Outdated Show resolved Hide resolved

test/per_file_coverage.sh Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Mar 31, 2021

lambdai added 2 commits April 2, 2021 15:42

Merge branch 'main' into errorflagref

d24a43d

address comment

b2e470f

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Apr 3, 2021

repokitteh-read-only bot added the waiting label Apr 4, 2021

clang-format-11

e4239f1

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Apr 5, 2021

lambdai added 4 commits April 5, 2021 12:40

add uint32accessor test

4253e4a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

upstreamsslconnection

ff58bdf

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

revert unnecessary

91e4a6f

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

runformat

98dc990

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz reviewed Apr 7, 2021

View reviewed changes

source/common/stream_info/utility.cc Outdated Show resolved Hide resolved

source/common/stream_info/utility.cc Show resolved Hide resolved

address Joshua comment

74289ce

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

jmarantz reviewed Apr 7, 2021

View reviewed changes

source/common/stream_info/utility.cc Outdated Show resolved Hide resolved

jmarantz previously approved these changes Apr 8, 2021

View reviewed changes

repokitteh-read-only bot assigned mattklein123 Apr 8, 2021

mattklein123 requested changes Apr 8, 2021

View reviewed changes

source/common/stream_info/utility.cc Outdated Show resolved Hide resolved

source/common/stream_info/utility.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Apr 8, 2021

update comment

479a80c

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai dismissed jmarantz’s stale review via 479a80c April 9, 2021 05:31

repokitteh-read-only bot added waiting and removed waiting labels Apr 9, 2021

use magic number 16

2585c65

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Apr 9, 2021

mattklein123 reviewed Apr 9, 2021

View reviewed changes

source/common/stream_info/utility.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Apr 9, 2021

remove traits header

9852e1f

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Apr 9, 2021

mattklein123 approved these changes Apr 9, 2021

View reviewed changes

mattklein123 merged commit 56d3f8a into envoyproxy:main Apr 9, 2021

lambdai deleted the errorflagref branch April 9, 2021 23:46

Monkeyanator pushed a commit to Monkeyanator/envoy that referenced this pull request Apr 20, 2021

tech debt: simplify access log code by adding a ALL_RESPONSE_STRING_F…

42297a6

…LAGS (envoyproxy#15580) Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Conversation

lambdai commented Mar 19, 2021

Uh oh!

lambdai commented Mar 20, 2021

Uh oh!

Uh oh!

jmarantz commented Mar 24, 2021

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz commented Mar 31, 2021

Uh oh!

jmarantz commented Apr 4, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Apr 8, 2021

Uh oh!

repokitteh-read-only bot commented Apr 8, 2021

Uh oh!

alyssawilk commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Apr 8, 2021

Uh oh!

alyssawilk commented Apr 8, 2021

Uh oh!

htuch commented Apr 8, 2021

Uh oh!

repokitteh-read-only bot commented Apr 8, 2021

Uh oh!

htuch commented Apr 8, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

itayd commented Apr 9, 2021

Uh oh!

lambdai commented Apr 9, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alyssawilk commented Apr 8, 2021 •

edited

Loading