Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

access log: add JSON logging mode #4681

Closed

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 <[email protected]>
@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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
Signed-off-by: Rama <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
Signed-off-by: xuzhonghu <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
… unordered container (envoyproxy#4573)

Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
… 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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
…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 [email protected]

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

Signed-off-by: Snow Pettersen <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
… in one case testing error-message throttling (envoyproxy#4591)

Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
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 [email protected]

Risk Level: Medium, new extension
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a
Signed-off-by: Snow Pettersen <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
Signed-off-by: Lizan Zhou <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
…ix (envoyproxy#4587)

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

Signed-off-by: Andres Guedez <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
Signed-off-by: Venil Noronha <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
)

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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
This allows configuring a gRPC retry polic which retries on internal
(13) responses.

Signed-off-by: Snow Pettersen <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
…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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
…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 <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
…om google::protobuf to Protobuf

Signed-off-by: Aaltan Ahmad <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
Signed-off-by: Aaltan Ahmad <[email protected]>
@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.