-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
ip tagging: remember tags as builtins #10856
ip tagging: remember tags as builtins #10856
Conversation
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
cc: @jmarantz |
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.
Thanks for doing this; one basic question.
I think the question didn't make it through... |
hit_(stat_name_set_->add("hit")), no_hit_(stat_name_set_->add("no_hit")), | ||
total_(stat_name_set_->add("total")) { | ||
no_hit_(stat_name_set_->add("no_hit")), total_(stat_name_set_->add("total")), | ||
unknown_tag_(stat_name_set_->add("unknown_tag.hit")) { |
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.
I had thought what we could do was walk through the tags and add all of them as builtins so we don't map them all to unknown
. Was there some reason you didn't want to do it that way?
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.
Eh?
I am doing that, in the loop that discovers the tags... the unknown_tag_
is needed because getBuiltin()
needs a fallback parameter...
tag_data.emplace_back(ip_tag.ip_tag_name(), cidr_set); | ||
stat_name_set_->rememberBuiltin(absl::StrCat(ip_tag.ip_tag_name(), ".hit")); |
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.
@jmarantz here we add each tag...
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.
Sorry; my bad. OK this looks great! Thanks!
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.
@envoyproxy/senior-maintainers
tag_data.emplace_back(ip_tag.ip_tag_name(), cidr_set); | ||
stat_name_set_->rememberBuiltin(absl::StrCat(ip_tag.ip_tag_name(), ".hit")); |
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.
Sorry; my bad. OK this looks great! Thanks!
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.
Thanks!
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com> Signed-off-by: pengg <pengg@google.com>
Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com