stats: Guard regex lookups with substring searches where possible #2693
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,8 +69,9 @@ std::string Utility::sanitizeStatsName(const std::string& name) { | |
| return stats_name; | ||
| } | ||
|
|
||
| TagExtractorImpl::TagExtractorImpl(const std::string& name, const std::string& regex, | ||
| const std::string& substr) | ||
| : name_(name), prefix_(std::string(extractRegexPrefix(regex))), substr_(substr), | ||
| regex_(RegexUtil::parseRegex(regex)) {} | ||
|
|
||
| std::string TagExtractorImpl::extractRegexPrefix(absl::string_view regex) { | ||
|
|
@@ -93,7 +94,8 @@ std::string TagExtractorImpl::extractRegexPrefix(absl::string_view regex) { | |
| } | ||
|
|
||
| TagExtractorPtr TagExtractorImpl::createTagExtractor(const std::string& name, | ||
| const std::string& regex, | ||
| const std::string& substr) { | ||
|
|
||
| if (name.empty()) { | ||
| throw EnvoyException("tag_name cannot be empty"); | ||
|
|
@@ -103,13 +105,18 @@ TagExtractorPtr TagExtractorImpl::createTagExtractor(const std::string& name, | |
| throw EnvoyException(fmt::format( | ||
| "No regex specified for tag specifier and no default regex for name: '{}'", name)); | ||
| } | ||
| return TagExtractorPtr{new TagExtractorImpl(name, regex, substr)}; | ||
| } | ||
|
|
||
| bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector<Tag>& tags, | ||
| IntervalSet<size_t>& remove_characters) const { | ||
| PERF_OPERATION(perf); | ||
|
|
||
| if (!substr_.empty() && stat_name.find(substr_) == std::string::npos) { | ||
|
Member
There was a problem hiding this comment. Can you add a simple test case to isolate this new portion of the logic and ensure it works as expected. The code is already covered by the regex test cases, but adding a separate test will isolate the functionality from the tests that check the regexes, themselves. See tests in https://github.com/envoyproxy/envoy/blob/master/test/integration/stats_integration_test.cc and https://github.com/envoyproxy/envoy/blob/master/test/common/stats/stats_impl_test.cc#L88 for other tests that test specific parts of the extraction process.
Contributor
Author
There was a problem hiding this comment. done. Broke out substrMismatch as a separate method to make it easier to test. |
||
| PERF_RECORD(perf, "re-skip-substr", name_); | ||
| return false; | ||
| } | ||
|
|
||
| std::smatch match; | ||
| // The regex must match and contain one or more subexpressions (all after the first are ignored). | ||
| if (std::regex_search(stat_name, match, regex_) && match.size() > 1) { | ||
|
|
@@ -181,7 +188,8 @@ int TagProducerImpl::addExtractorsMatching(absl::string_view name) { | |
| int num_found = 0; | ||
| for (const auto& desc : Config::TagNames::get().descriptorVec()) { | ||
| if (desc.name_ == name) { | ||
| addExtractor( | ||
| Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, desc.substr_)); | ||
| ++num_found; | ||
| } | ||
| } | ||
|
|
@@ -241,7 +249,8 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon | |
| if (!config.has_use_all_default_tags() || config.use_all_default_tags().value()) { | ||
| for (const auto& desc : Config::TagNames::get().descriptorVec()) { | ||
| names.emplace(desc.name_); | ||
| addExtractor( | ||
| Stats::TagExtractorImpl::createTagExtractor(desc.name_, desc.regex_, desc.substr_)); | ||
| } | ||
| } | ||
| return names; | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
".ssl.ciphers."like in the other examples?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That one was an omission, because I had some trouble with ".http." below and I wound up backing off the inclusion of the "." in some places where it was OK.
However I managed to sort out my issues with ".http." by exploiting the fact that I can now have two separate regexes for the same tag.