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: refactored SubstitutionFormatParser::parseCommand #16121

Merged
merged 10 commits into from
May 4, 2021

Conversation

cpakulski
Copy link
Contributor

Commit Message:
refactored SubstitutionFormatParser::parseCommand

Additional Description:
This PR addresses few items:

  • tech debt: there was a TODO comment that parsing access_log command should use string_view to avoid copying. After using string_view SubstitutionFormatParser::parseCommand method is around 40% faster. Microbenchmark reports the following results:

Benchmark Time CPU Iterations
BM_FormatterCommandParsingOld 2194 ns 2194 ns 319163
BM_FormatterCommandParsingNew 1260 ns 1260 ns 554762

  • SubstitutionFormatParser::parseCommand has been implemented using variadic template. This is done in preparation for issue access_log: support for logging route/listener/cluster metadata #3006. The idea has been proposed on envoy-dev slack on April 14th 2021. In a nutshell, there will be one tag for all metadata in form METADATA(TYPE:NAMESPACE:KEY):Z. Notice that there is additional tag TYPE. Using Variadic template for parsing commands will automatically address the presence of extra tag.

Risk Level: Low
Testing: Added new unit tests and microbenchmark
Docs Changes: Not required
Release Notes: Not required

Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
@cpakulski cpakulski marked this pull request as ready for review April 22, 2021 20:42
@cpakulski
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16121 (comment) was created by @cpakulski.

see: more, trace.

@cpakulski
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16121 (comment) was created by @cpakulski.

see: more, trace.

@yanavlasov yanavlasov self-assigned this Apr 30, 2021
@yanavlasov
Copy link
Contributor

Thanks, this looks really good. Could you add some negative tests as well, i.e. when command is malformed to test parsing error handling, please?

@yanavlasov
Copy link
Contributor

/wait

@yanavlasov yanavlasov merged commit b9d21f0 into envoyproxy:main May 4, 2021
@cpakulski
Copy link
Contributor Author

@yanavlasov Thanks for reviewing and merging.

@cpakulski cpakulski deleted the issue/3006 branch May 4, 2021 15:11
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 5, 2021
gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
@rgs1
Copy link
Member

rgs1 commented May 10, 2021

FYI this broke our internal formatter extensions:

16:48:27 pinterest/formatters/dynamic_metadata_unquoted/formatter.cc:54:5: error: no matching function for call to 'parseCommand'
16:48:27     Envoy::Formatter::SubstitutionFormatParser::parseCommand(token, start, ":", filter_namespace,
16:48:27     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Not a huge deal, but something to take in account code that might be relied upon by extensions...

@cpakulski
Copy link
Contributor Author

@rgs1 I apologize for that. I could probably leave original methods with original signatures and map it to the new ones.
I am guessing that the problem is with different order of parameters.

@rgs1
Copy link
Member

rgs1 commented May 10, 2021

@cpakulski no worries at all, the change was easy. Mostly just posted for visibility, since eventually formatter extensions might become more popular (I suspect they are not heavily used right now).

lizan pushed a commit that referenced this pull request Aug 6, 2021
This PR addresses several issues:
1. MetadataFormatter base class takes a pointer to a function to access specific type of metadata. All other methods operating on internals of metadata structure are moved to base class. This simplified adding new types of metadata.
2. new type of  access_log METADATA tag is introduced to handle all types of metadata. It is implemented as formatter extension. It handles existing tags DYNAMIC_METADATA and CLUSTER_METADATA, so two ways of accessing dynamic and cluster metadata are allowed now. Maybe we should obsolete DYNAMIC_METADATA and CLUSTER_METADATA in the future?
3. Adds handler for ROUTE metadata. See issue #3006.

Follow up to #16121

Risk Level: Low
Testing: Unit tests.
Docs Changes: Yes.
Release Notes: Yes.
Platform Specific Features: No

Signed-off-by: Christoph Pakulski <[email protected]>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
This PR addresses several issues:
1. MetadataFormatter base class takes a pointer to a function to access specific type of metadata. All other methods operating on internals of metadata structure are moved to base class. This simplified adding new types of metadata.
2. new type of  access_log METADATA tag is introduced to handle all types of metadata. It is implemented as formatter extension. It handles existing tags DYNAMIC_METADATA and CLUSTER_METADATA, so two ways of accessing dynamic and cluster metadata are allowed now. Maybe we should obsolete DYNAMIC_METADATA and CLUSTER_METADATA in the future?
3. Adds handler for ROUTE metadata. See issue envoyproxy#3006.

Follow up to envoyproxy#16121

Risk Level: Low
Testing: Unit tests.
Docs Changes: Yes.
Release Notes: Yes.
Platform Specific Features: No

Signed-off-by: Christoph Pakulski <[email protected]>
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.

4 participants