Skip to content

stats: Add iterate methods for tags.#6764

Merged
jmarantz merged 8 commits intoenvoyproxy:masterfrom
jmarantz:tag-iter
May 7, 2019
Merged

stats: Add iterate methods for tags.#6764
jmarantz merged 8 commits intoenvoyproxy:masterfrom
jmarantz:tag-iter

Conversation

@jmarantz
Copy link
Contributor

@jmarantz jmarantz commented May 1, 2019

Description: With #6161, the tags are no longer held in memory as a vector, but that structure has to be created when tags() is called. If a caller's goal is to find a tag by name, it's more efficient to capture the tag-name in a StatName in a context, and then iterate over the tags as StatNames looking for it. This provides two variants on an iterate() method to make this easier: the very efficient iterateTagStatNames(), and a slightly more compute-intensive version that iterates over const Tag& objects.
Risk Level: low
Testing: //test/common/stats/... -- note that tags() is already called in various tests, and that depends on all the new method bodies.
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz requested a review from eziskind May 1, 2019 02:28
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz jmarantz marked this pull request as ready for review May 1, 2019 02:31
@jmarantz
Copy link
Contributor Author

jmarantz commented May 1, 2019

@eziskind this is intended to provide a higher-performing way for stats sinks to look for a particular stat, rather than building a temp array of string-pairs, per f2f discussion.

Copy link
Contributor

@eziskind eziskind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add unit tests - otherwise LGTM, Thanks!

@eziskind
Copy link
Contributor

eziskind commented May 1, 2019

Add unit tests - otherwise LGTM, Thanks!

Ah, I see your comment about how the new methods are already tested with unit tests that use "tags". So I guess you don't need to add more

@zuercher zuercher self-assigned this May 1, 2019
jmarantz added 3 commits May 1, 2019 17:49
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Copy link
Contributor Author

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eziskind actually upon double-checking I don't think there really were good unit-tests for tags(), so I added a few variants in a new test metric_impl_test.cc. It did have indirect coverage but not simple unit-tests, which it deserves.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Contributor Author

jmarantz commented May 2, 2019

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #6764 (comment) was created by @jmarantz.

see: more, trace.

jmarantz added 2 commits May 6, 2019 22:58
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
zuercher
zuercher previously approved these changes May 7, 2019
@jmarantz
Copy link
Contributor Author

jmarantz commented May 7, 2019

Thanks for the re-approve @zuercher :)

@jmarantz jmarantz merged commit 3786f96 into envoyproxy:master May 7, 2019
@jmarantz jmarantz deleted the tag-iter branch May 7, 2019 16:47
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.

3 participants