Skip to content

access log: add JSON logging mode#4681

Closed
aa-stripe wants to merge 70 commits intoenvoyproxy:masterfrom
aa-stripe:aa-stripe/2692-json-logging
Closed

access log: add JSON logging mode#4681
aa-stripe wants to merge 70 commits intoenvoyproxy:masterfrom
aa-stripe:aa-stripe/2692-json-logging

Conversation

@aa-stripe
Copy link
Contributor

@aa-stripe aa-stripe commented Oct 10, 2018

These are the code changes to add a JSON access logging mode. I am working on the docs/release notes, but wanted to get started on code review!

Description:

  • Add a new config option under access_log called json_format. This is a single level dictionary that contains strings as keys, and envoy access log format specifiers (such as %PROTOCOL%) as values. The specifiers will be replaced with actual values at logging time. I call this dictionary the "format dictionary" (as opposed to "format string").
  • You can specify only one of format (format string) or json_format (format dictionary). If neither are there, we fall back to the default string format.
  • Add the correct plumbing inside the configuration parsing to handle this.
  • Add a new access log formatter class that is instantiated with the format dictionary. It maintains the mapping of dictionary keys to loggers
  • Create a new class called FormatterProvider, to distinguish things that actually extract the information from a request. The things that combine together a bunch of FormatterProviders are still called Formatters. This is primarily a semantic/naming difference, but imo these are two conceptually separate things. There is, however no API difference, and if people are truly opposed to this, I could just merge them back into one Formatter class. This also provides a better foundation for adding more log formats in the future.
  • At present, only one specifier per key in the format dictionary is allowed. This is because the whole point of JSON logging is to make logs easily machine-parseable. If you can include multiple formats in the same field, then you'll be right back to parsing those manually
  • At present, only top-level keys are allowed in the format dictionary. This is validated at config load time. In the future, we can expand this to have nested dictionaries.

Risk Level:
Low. It's an optional feature that has to be explicitly enabled.

Testing:
Unit testing for the actual formatter, and config load. Also manually tested using an example config file.

Docs:
Will have to add docs. Could someone please advise on how to preview the docs so that I can make sure they are good as I write them?

Release Notes:
Will have to add Release notes

Fixes #2692

Signed-off-by: Aaltan Ahmad <aa@stripe.com>
@aa-stripe aa-stripe force-pushed the aa-stripe/2692-json-logging branch from 97e0dfd to a54ea42 Compare October 10, 2018 21:02
@aa-stripe
Copy link
Contributor Author

Working on getting this building again. The recent RequestInfo->StreamInfo change messed things up a little.

lizan and others added 27 commits October 10, 2018 17:38
Description:
Seems it causing problem frequently, disable it to see if it helps.

Risk Level: Low
Testing: CI
Docs Changes:
Release Notes:
Fixes envoyproxy#4407

Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Rama <rama.rao@salesforce.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: xuzhonghu <xuzhonghu@huawei.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
…voyproxy#4560)

This patch reintroduces PR envoyproxy#4217.

Signed-off-by: Tony Allen <tallen@lyft.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
This is the remove counterpart to envoyproxy#4220.

Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=10737.

Risk Level: Low
Testing: Unit test and corpus entry added.

Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
… unordered container (envoyproxy#4573)

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
As documented in envoyproxy#4541 it appears that in the H2 case both the codec, in [Client|Server]ConnectionImpl::newStream, and the http connection manager in ConnectionManagerImpl::newStream call high watermark callbacks when a new stream is created. This results in double counting for tcp connection level blocks in the H2 path and connection stalls.

This PR removes the watermark callbacks from the http connection manager, adds the to the http/1.x codec for consistency, then adds an assert in the http connection manager to theoretically regression test other codecs.

Risk Level: High
Testing: new integration test
Fixes: envoyproxy#4541

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
This PR reverts envoyproxy#4382. When deploying at Lyft we noticed crashes on here where we might be derefencing the connection_stats_ pointer after the point has been reset.

Note: this PR keeps the changes to the API made in the original PR but tags the field as not implemented. This is what we have done in the past for reverts that involve changes that change the API.

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
… key (envoyproxy#4577)

- now repository_impl supports overriding the repository location key
which can be used to simulate repository aliases
- used the new feature for the protobuf_cc repository
- it is now posible to override the protobuf repository location on the
command line and the build to work correctly

Signed-off-by: Georgi Dimitrov <gdimitrov@vmware.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
…nvoyproxy#4533)

Ensures that when priority load is distributed between priorities and
there is a rounding error the remaining value is given to a healthy
priority. Previously, this remainder would always be given to priority
0, which would result in a completely unhealthy priority having non-zero
load.

If P0 was selected due to a rounding error but was otherwise unhealthy, the
request would fail with UH as there are no healthy hosts available in the selected
priority (unless panic mode was triggered).

Signed-off-by: Snow Pettersen snowp@squareup.com

Risk Level: Medium
Testing: Unit test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
…4506)

ref: envoyproxy#4390

Signed-off-by: Taiki Ono <taiki-ono@cookpad.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Gary Brown <gary@brownuk.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
… in one case testing error-message throttling (envoyproxy#4591)

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
update doc for jwt_authn http filter

format config.proto comment for doc
add a new rst file: docs/root/configuration/http_filters/jwt_authn_filter.rst

Risk Level: None
Docs Changes: Yes

Signed-off-by: Wayne Zhang <qiwzhang@google.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
…oyproxy#4592)

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Implements a RetryPriority which will keep track of attempted
priorities and attempt to route retry requests to other priorities. The
update frequency is configurable, allowing multiple requests to hit each
priority if desired.

As a fallback, when no healthy priorities remain, the list of attempted
priorities will be reset and a host will selected again using the
original priority load.

Extracts out the recalculatePerPriorityState from LoadBalancerBase to
recompute the priority load with the same code used by the LB.

Signed-off-by: Snow Pettersen snowp@squareup.com

Risk Level: Medium, new extension
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Dhi Aurrahman <dio@tetrate.io>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
…ix (envoyproxy#4587)

Re-enable the changes reverted in 9d32e5c, which were originally merged as part of envoyproxy#4382.

Signed-off-by: Andres Guedez <aguedez@google.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Moving all our integration test filters to one directory and making AddTrailersStreamFilter depend on PassThroughFilter to avoid a bunch of functions that ::Continue

Risk Level: Low (test only)
Testing: tests still pass.
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
This is due to bazelbuild/buildtools#383

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Venil Noronha <veniln@vmware.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Description:
This PR updates the documentation related to the recent rename of RequestInfo to StreamInfo. Missed these changes in envoyproxy#4503. Also, see envoyproxy#4500 for further info.

Risk Level: Low
Testing: N/A
Docs Changes: Multiple
Release Notes: N/A

Signed-off-by: Venil Noronha <veniln@vmware.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Description: While working on certs changes, I realized that memory proto doc links were missing. This PR adds them.
Risk Level: Low

Signed-off-by: Rama <rama.rao@salesforce.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
perlun and others added 20 commits October 10, 2018 17:42
As of envoyproxy#3117, Lua support is no longer experimental.

Signed-off-by: Per Lundberg <perlun@gmail.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Lizan Zhou <lizan@tetrate.io>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
)

Uses the correct header key for grpc retry policy.  The test does not use the headers, but it's still good to have the right ones in there.

Risk Level: Low

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
This allows configuring a gRPC retry polic which retries on internal
(13) responses.

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
…nvoyproxy#4670)

Previously this was set to Empty, which caused config parsing to fail
with

message=Unable to parse JSON as proto (INVALID_ARGUMENT:: invalid name update_frequency: Cannot find field.): {"update_frequency":2}

Risk Level: Low
Testing: n/a
Docs Changes: n/a

Signed-off-by: Snow Pettersen <snowp@squareup.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
…oyproxy#4671)

Risk Level: Medium
Testing: updated unit tests
Docs Changes: nope
Release Notes: called out a warning in the release notes
Fixes: envoyproxy#3611

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
…om google::protobuf to Protobuf

Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
@alyssawilk
Copy link
Contributor

Yeah, I think your branch may have some screwed up state. Feel free to either ping when it's ready for review, or if you need to close it out and start from scratch that's fine too.

@zuercher
Copy link
Member

You can build the docs via ./docs/build.sh. I believe it will install the necessary python packages automatically. The docs are generated into generated/docs/.

@aa-stripe
Copy link
Contributor Author

Yeah, I fixed the StreamInfo issue, but things got real screwed up when I did the signoff rebase. I'm trying to fix this now. Thanks @zuercher, I'll get that running locally

@aa-stripe
Copy link
Contributor Author

Closing in favor of #4693

@aa-stripe aa-stripe closed this Oct 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.