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

stats: support default stats tags with fixed value #2357

Merged
merged 25 commits into from Jan 22, 2018

Conversation

Projects
None yet
5 participants
@taiki45
Member

taiki45 commented Jan 12, 2018

Description:
Support default stats tag which will be added to all metrics. Typical use case is
identifying Envoy instances with the tag.

The previous disscussions were:

  • we don't use environment variable extraction due to configuration inconsistency. ref: #1975 (comment)
  • we don't add any more ad-hoc CLI flag. ref: #2264 (review)
  • generic override CLI flag can be implemented to overwrite this fixed tag value option but not in this PR. ref: #2264 (comment)

So just implement fixed stats tag value option and leave CLI override feature to #2269.

Risk Level: Medium
New features that are not enabled by default.

Testing:
unit test and integration test should cover this. Also munally tested with with statsd_exporter v0.5.0 and Prometheus v2.0.0.

Docs Changes:
envoyproxy/data-plane-api#416

Release Notes:
Added.

API Changes:
envoyproxy/data-plane-api#416

Fixes #1975

taiki45 added some commits Jan 11, 2018

stats: support default stats tags with fixed value
The default tags will be added to all stats. Typical use case is
identifying Envoy instances with the tag.

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Add missing code document
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Pass IPv6 test cases
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
@htuch
Member

htuch left a comment

Thanks. @mrice32 can you take a pass? Cheers.

* all metrics.
* @param bootstrap bootstrap proto.
*/
static std::vector<Stats::Tag> createTags(const envoy::api::v2::Bootstrap& bootstrap);

This comment has been minimized.

@htuch

htuch Jan 12, 2018

Member

Should this be internal to utility.cc and just live in the anonymous namespace?

This comment has been minimized.

@taiki45

taiki45 Jan 13, 2018

Member

I'm not familiar but since this function needs to be called from server/server.cc, I thought this function needs to be published. Is that wrong?

Possibly will be refactored with #2357 (comment).

This comment has been minimized.

@taiki45

taiki45 Jan 17, 2018

Member

Refactored with e059698 not to expose these functions. Good point!

@@ -87,6 +87,10 @@ void ThreadLocalStoreImpl::releaseScopeCrossThread(ScopeImpl* scope) {
}

std::string ThreadLocalStoreImpl::getTagsForName(const std::string& name, std::vector<Tag>& tags) {
if (default_tags_ != nullptr) {

This comment has been minimized.

@htuch

htuch Jan 12, 2018

Member

Why is this now necessary?

This comment has been minimized.

@taiki45

taiki45 Jan 13, 2018

Member

Since we have to initialize stats stores before we ready the default_tags, stats stores have their default_tags_ fields as a point (const std::vector<Tag>*). Pointers can be null, so I thought we need check nullability of the pointer. The tag_extractors_ also checks the nullability below. Still I'm not sure it's not wrong.

This comment has been minimized.

@taiki45

taiki45 Jan 17, 2018

Member

Refactored with e059698 to stop using pointer approach. Instead, use null object pattern to ensure they always exist.

@dnoe dnoe requested review from dnoe and mrice32 Jan 12, 2018

commit = "f268484d221509b175c46cce6c4cc3593a0ede5e",
remote = "https://github.com/envoyproxy/data-plane-api",
commit = "07b79a51679558d6460fd2785b7206d102869f4d",
remote = "https://github.com/taiki45/data-plane-api",

This comment has been minimized.

@dnoe

dnoe Jan 12, 2018

Contributor

Looks like this snuck in by accident.

This comment has been minimized.

@mrice32

mrice32 Jan 12, 2018

Member

I'm assuming this was purposeful to align with envoyproxy/data-plane-api#416 to pass CI. What's the expected workflow for synchronized changes to both repos?

This comment has been minimized.

@dnoe

dnoe Jan 13, 2018

Contributor

Ah, yes, that makes sense. Normally you'd get your change reviewed and merged over at data-plane-api, then update the hash in your PR here (if someone else updates it first past your changes in data-plane-api, just take the newer hash when you merge master).

It's fine to push a PR for review pointing at the author's repo to get early feedback, but obviously it needs to get updated before the PR can be merged. It's probably good practice to comment on your own PR to mark it as needing work.

This comment has been minimized.

@taiki45

taiki45 Jan 13, 2018

Member

Yes, I read the workflow at https://github.com/envoyproxy/data-plane-api/blob/master/CONTRIBUTING.md#api-changes but leaving the comment encourages reviews. I will do that, thanks!


if (!bootstrap.stats_config().has_use_all_default_tags() ||
bootstrap.stats_config().use_all_default_tags().value()) {
for (const std::pair<std::string, std::string>& default_tag :

This comment has been minimized.

@dnoe

dnoe Jan 12, 2018

Contributor

Using const auto& for this might make it easier to read.

}
}

for (const envoy::api::v2::TagSpecifier& tag_specifier : bootstrap.stats_config().stats_tags()) {

This comment has been minimized.

@dnoe

dnoe Jan 12, 2018

Contributor

const auto& is also probably a good choice here.

};
std::vector<Stats::TagExtractorPtr>
Utility::createTagExtractors(const envoy::api::v2::Bootstrap& bootstrap) {
std::vector<Stats::TagExtractorPtr> tag_extractors;

This comment has been minimized.

@dnoe

dnoe Jan 12, 2018

Contributor

It looks like you know how many items will go into tag_extractors. You can use this with reserve to avoid any costly reallocations:

tag_extractors.reserve(default_tag_size + bootstrap_tags_size)

This comment has been minimized.

@taiki45

taiki45 Jan 13, 2018

Member

Nice, I didn't know that. I roughly estimate the size but should I get precise size?
d3b5e7f

std::vector<Stats::Tag> Utility::createTags(const envoy::api::v2::Bootstrap& bootstrap) {
std::vector<Stats::Tag> tags;

for (const envoy::api::v2::TagSpecifier& tag_specifier : bootstrap.stats_config().stats_tags()) {

This comment has been minimized.

@dnoe

dnoe Jan 12, 2018

Contributor

Use const auto& for these long type names.

* Envoy can have both pre-defined tag names in well_known_names and user
* provided tag names through bootstrap configuration. Check the confilict of
* all the tag names to avoid unexpected tag name overwriting. If found a
* confilict, throw an exception.

This comment has been minimized.

@dnoe

dnoe Jan 12, 2018

Contributor

There's a typo ("confilict"). It also might be more readable to alter the text a bit:

"Check all tag names for conflicts to avoid unexpected tag name overwriting. If a conflict is found, throw an exception."

* all the tag names to avoid unexpected tag name overwriting. If found a
* confilict, throw an exception.
* @param bootstrap bootstrap proto.
*/

This comment has been minimized.

@dnoe

dnoe Jan 12, 2018

Contributor

It might not be consistently applied across the Envoy code base but this is a good opportunity to use the @throws doxygen tag to specify what exception type and when it is thrown.

@@ -70,6 +70,7 @@ class ThreadLocalStoreImpl : public StoreRoot {
void setTagExtractors(const std::vector<TagExtractorPtr>& tag_extractors) override {
tag_extractors_ = &tag_extractors;
}
void setDefaultTags(const std::vector<Tag>& tags) override { default_tags_ = &tags; }

This comment has been minimized.

@dnoe

dnoe Jan 12, 2018

Contributor

This seems risky. What's the lifespan of tags guaranteed to be? I think it is OK because tags has the lifespan of the server. But any time we are saving a pointer to a value passed by reference it's a good idea to document what controls the lifecycle of that memory.

This comment has been minimized.

@taiki45

taiki45 Jan 13, 2018

Member

Yeah, a server hold the value so the lifespan will be same as the server's one. Looks good with this comment? a94dbda

This comment has been minimized.

@taiki45

taiki45 Jan 17, 2018

Member

Finally stopped this approach and change thread local stores to have ownership of those pointers in e059698.

@dnoe

This comment has been minimized.

Contributor

dnoe commented Jan 12, 2018

@mrice32 Can you also take a look at this? Thanks.

@mrice32
Member

mrice32 left a comment

This looks great! Just a few nits and one comment about possibly changing the design a bit.

std::vector<Stats::TagExtractorPtr> tag_extractors;

// Ensure no tag names are repeated.
void Utility::detectTagNameConflict(const envoy::api::v2::Bootstrap& bootstrap) {

This comment has been minimized.

@mrice32

mrice32 Jan 12, 2018

Member

Not sure what others think about this, but is there a way we could get the name conflict detection as a part of the creation of TagExtractors? We could do that in a few ways:

  1. We could create a stateful class for doing this initial config processing, keeping track of TagExtractors, and taking care of doing stat creation-time processing (like getTagsForName() in ThreadLocalStore). This is probably the elegant and more maintainable solution because it has the benefit of reducing complexity in other classes, too.
  2. We could pass mutable state between these utility methods. This would be a bit ugly, but it would get rid of the nearly repeated code.

This comment has been minimized.

@taiki45

taiki45 Jan 13, 2018

Member

Agree. I was also thiking how we generalize tag extractors and default tags. It's off-topic but I started with creating tag extrators which do match every stat names but it was not good idea.

(1) sounds good to me, so I'll try (1). Please tell me the possible name of the class if you have ideas. The class will consume bootstrap configurations and build it-self, then create proper tags when a stats name will be given. Maybe TagProvider or TagProducer?

This comment has been minimized.

@mrice32

mrice32 Jan 16, 2018

Member

Yeah, I thought about the idea of a generalized tag extractor as well, and for performance reasons, I agree with you that it's probably best to not do it that way.

Great! Yeah, I'm not too great with naming, but both of those seem reasonable to me.

This comment has been minimized.

@taiki45

taiki45 Jan 17, 2018

Member

Implemented (1) in e059698. Please note I didn't move bootrap config consuming task from Config::Utility bacause (a). the TagProducer (and its factory) living in the stats component shouldn't depend on Bootrap config (b) the consuming task is not so big from the both points of code size and responsibility. If you have a better place for moving the task, I'm glad to implement that.

@@ -122,6 +123,7 @@ class ThreadLocalStoreImpl : public StoreRoot {
ScopePtr default_scope_;
std::list<std::reference_wrapper<Sink>> timer_sinks_;
const std::vector<TagExtractorPtr>* tag_extractors_{};
const std::vector<Tag>* default_tags_{};

This comment has been minimized.

@mrice32

mrice32 Jan 12, 2018

Member

Adding a class as mentioned in 1 above would mean that this class doesn't have to keep track of two separate sets of related data and know how to process it.

tag_extractors_ = Config::Utility::createTagExtractors(bootstrap);
Config::Utility::createTags(bootstrap);

This comment has been minimized.

@mrice32

mrice32 Jan 12, 2018

Member

nit: just for consistency, you can remove tag_extractors_ from this file as well since it appears that it isn't being used.

EXPECT_THROW_WITH_MESSAGE(Utility::detectTagNameConflict(bootstrap), EnvoyException,
fmt::format("Tag name '{}' specified twice.", "test.x"));

// Also should raise an error When user defined tag name conflicts with Envoy's default tag names.

This comment has been minimized.

@mrice32

mrice32 Jan 12, 2018

Member

nit: When -> when

taiki45 added some commits Jan 13, 2018

Drop the "default" term from the raw release notes
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Comment about pinning to the temporal fork
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Use `auto` in proper places
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Avoid costly reallocations
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Better code document
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Comment about lifecycle of pointers
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Fix typo: When -> when
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Merge remote-tracking branch 'origin/master' into default-stats-tag
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Make TagExtractorPtr to const
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Refactoring: add TagProducer which manages tags
Collect responsibility of the stats tag management to the newly created
class, TagProducer.  A set of TagExtractor and a set of default Tag will
be handled via TagProducer class.

I'm also looking proper places to extract bootstrap config handling
around stats tag from Config::Utility. Since the code size and
responsibility are limited, leave the config handling as it is for this
time.

For tests, I moved some of test cases from utility unit test to stats
integration test. This is because we can't (or shouldn't) test the
internal functions inside Config::Utility. In addition, some of them are
actually testing tag extracor behaviors, so extract them from utility
unit test.

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
return tag_extractors;
}

Stats::TagProducerPtr Utility::createTagProducer(const envoy::api::v2::Bootstrap& bootstrap) {

This comment has been minimized.

@mrice32

mrice32 Jan 17, 2018

Member

Do you think it makes sense to encapsulate all of this config interpretation logic in the constructor for TagProducerImpl?

This comment has been minimized.

@taiki45

taiki45 Jan 17, 2018

Member

I don't think that. In my opinion, logics which handle a bootstrap config should be in Config or Server. I think it makes sense to extract these logics to somewhere in Config namespace. WDYT?

This comment has been minimized.

@mrice32

mrice32 Jan 17, 2018

Member

If you look at virtual clusters or routes and others, there are a bunch of examples of v2 api objects being passed to to parameterize their representation in Envoy. I think this method of encapsulation is preferred in Envoy. Also, just a note, we should only need to pass the StatsConfig object rather than the entire bootstrap config.

I prefer this approach as well, especially when there's only one class in Envoy that represents a particular config object (in this case StatsConfig only configures TagProducer). I think it allows us to contain the component-specific logic within specific classes and directories pertaining to that component rather than polluting the more general areas of the codebase with it. Also, it gives more flexibility when maintaining state - for instance, in this case you could write the default Tags and TagExtractors to member vectors rather than be forced to use the in/out semantics of a c-style function.

This is just my opinion, though. Others may feel differently.

This comment has been minimized.

@taiki45

taiki45 Jan 17, 2018

Member

+1, passing StatsConfig message or pieces of bootstrap config makes sense to me. How about the refacoring plan: Utility extracts stats proto messages from bootstrap message, then pass them to a factory of TagProducer (or direcly to TagProducerImpl).

This comment has been minimized.

@taiki45

taiki45 Jan 17, 2018

Member

Just a thought: I'm wondering doing the stateful initalization in the TagProducer will change it to a mutable object. I think it's better Entity objects keep their immutablility, so "building data in the other places and passing them into an Entiry object" approach isn't always bad.

hmm... after some thinking, using a stateful object to build tag extroctors and default tags seems better to me in this case. I'm just thiking to get both immutability of TagProducer and that stateful object for the config processing task.

This comment has been minimized.

@mrice32

mrice32 Jan 17, 2018

Member

I'm not exactly sure what you mean when you say mutable object? The object could still be passed around as const and all of TagProducer's interface methods can be const, so it won't allow anyone to modify any fields after construction.

If you're referring to making all the fields inside TagProducerImpl const, I'm not sure that's a big concern here, especially considering the added complexity that would be necessary to make this work.

This comment has been minimized.

@taiki45

taiki45 Jan 18, 2018

Member

Sorry for the confustion. I misunderstood I have to change the constbility of the object but it's wrong. Then the refactoring plan sounds great to me. Implemented it with f736311. Cloud this match to your idea?

This comment has been minimized.

@mrice32

mrice32 Jan 18, 2018

Member

Yep, I think this new design is great! I'll take another pass over the code and make comments.

taiki45 added some commits Jan 17, 2018

Remove bootstrap including from stats_impl
It seems we don't need it.

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Merge remote-tracking branch 'origin/master' into default-stats-tag
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Move the config consuming logic to the producer
For refactoring.

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
@mrice32
Member

mrice32 left a comment

Looks great! LGTM after comments.

void addDefaultExtractors(const envoy::api::v2::StatsConfig& config,
std::unordered_set<std::string>& names);

ExtractorsPtr tag_extractors_;

This comment has been minimized.

@mrice32

mrice32 Jan 18, 2018

Member

nit: Can we just make these vectors instead of pointers to vectors? They will always be non-nullptr right?

EXPECT_THROW_WITH_MESSAGE(
Utility::createTagExtractors(bootstrap), EnvoyException,
"No regex specified for tag specifier and no default regex for name: 'test_extractor'");
Utility::createTagProducer(bootstrap);

This comment has been minimized.

@mrice32

mrice32 Jan 18, 2018

Member

This test seems to just exist now to show that this method doesn't throw. Do you think we could add in a little logic to ensure the returned pointer is not nullptr and can be used (by maybe calling produceTags() on it or something)?

This comment has been minimized.

@taiki45

taiki45 Jan 18, 2018

Member

Yeah, feeling I'm missing something here. +1 to add some tests against the returned value.

TagProducerImpl::~TagProducerImpl() {}

std::string TagProducerImpl::produceTags(const std::string& name, std::vector<Tag>& tags) const {
if (!default_tags_->empty()) {

This comment has been minimized.

@mrice32

mrice32 Jan 18, 2018

Member

nit: do we need this if()? It should be fine to insert an empty vector, right?

This comment has been minimized.

@taiki45

taiki45 Jan 18, 2018

Member

Right. My concern was the cost of inserting an emptry vector.

taiki45 added some commits Jan 18, 2018

Drop if-branching, just insert an empty vector
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Stop holding member vars via unique_ptr
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Test the returned producer functionality a little
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
@mrice32
Member

mrice32 left a comment

LGTM, thanks!

@mattklein123
Member

mattklein123 left a comment

very nice, just a few nits.


return tag_extractors;
Stats::TagProducerPtr Utility::createTagProducer(const envoy::api::v2::Bootstrap& bootstrap) {
return Stats::TagProducerPtr{new Stats::TagProducerImpl(bootstrap.stats_config())};

This comment has been minimized.

@mattklein123

mattklein123 Jan 18, 2018

Member

nit: std::make_unique

This comment has been minimized.

@taiki45

This comment has been minimized.

@mattklein123

mattklein123 Jan 19, 2018

Member

We didn't have make_unique until we switched to c++14, so there are tons of places where it could be used. I wouldn't go out of your way, but in new cases I would use it.

}
}

TagProducerImpl::TagProducerImpl() {}

This comment has been minimized.

@mattklein123

mattklein123 Jan 18, 2018

Member

nit: del empty constructor and destructor (if default constructor is needed for some reason (?) just define in header file)

throw EnvoyException(fmt::format("Tag name '{}' specified twice.", tag_specifier.tag_name()));
}

// If no tag value are found, fallback to default regex to keep backward compatibility.

This comment has been minimized.

@mattklein123

mattklein123 Jan 18, 2018

Member

"no tag value is found"

void addDefaultExtractors(const envoy::api::v2::StatsConfig& config,
std::unordered_set<std::string>& names);

std::vector<TagExtractorPtr> tag_extractors_{};

This comment has been minimized.

@mattklein123

mattklein123 Jan 18, 2018

Member

nit {} not needed here and next line

void setTagExtractors(const std::vector<TagExtractorPtr>& tag_extractors) override {
tag_extractors_ = &tag_extractors;
void setTagProducer(TagProducerPtr&& tag_producer) override {
tag_producer_ = std::move(tag_producer);

This comment has been minimized.

@mattklein123

mattklein123 Jan 18, 2018

Member

If this is generally meant to be called once, I would add an ASSERT(tag_producer_ == nullptr); here.

This comment has been minimized.

@taiki45

taiki45 Jan 19, 2018

Member

We expects this to be called one. But tag_producer_ will never be nullptr due to this construcor https://github.com/envoyproxy/envoy/pull/2357/files#diff-4112b28a18267da2a53245cfbacaeb94R16.

This comment has been minimized.

@mattklein123

mattklein123 Jan 19, 2018

Member

OK no problem just skip it.

taiki45 added some commits Jan 19, 2018

Prefer std::make_unique
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
English grammar fix
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Remove unnecessary constructors and destructors
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Remove unnecessary initialization
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
tag_specifier.tag_name(), tag_specifier.regex()));

} else if (tag_specifier.tag_value_case() == envoy::api::v2::TagSpecifier::kFixedValue) {
default_tags_.emplace_back(Stats::Tag{tag_specifier.tag_name(), tag_specifier.fixed_value()});

This comment has been minimized.

@mrice32

mrice32 Jan 19, 2018

Member

nit: Can we use this notation: Stats::Tag{.name_ = tag_specifier.tag_name(), .value_ = tag_specifier.fixed_value()}?

This comment has been minimized.

@taiki45
Use member name notation
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
@mattklein123
Member

mattklein123 left a comment

sorry tiny nit that I just saw, otherwise LGTM

public:
virtual ~TagProducer() {}

/*

This comment has been minimized.

@mattklein123

mattklein123 Jan 21, 2018

Member

super nit: /**

/**
* Creates the set of stats tag extractors requested by the config and transfers ownership to the
* caller.
/*

This comment has been minimized.

@mattklein123

mattklein123 Jan 21, 2018

Member

same

TagProducerImpl(const envoy::api::v2::StatsConfig& config);
TagProducerImpl() {}

/*

This comment has been minimized.

@mattklein123

mattklein123 Jan 21, 2018

Member

same

Fix comment format
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

@mattklein123 mattklein123 merged commit 558431e into envoyproxy:master Jan 22, 2018

10 checks passed

DCO All commits have a DCO sign-off from the author
ci/circleci: asan Your tests passed on CircleCI!
Details
ci/circleci: build_image Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: format Your tests passed on CircleCI!
Details
ci/circleci: ipv6_tests Your tests passed on CircleCI!
Details
ci/circleci: mac Your tests passed on CircleCI!
Details
ci/circleci: release Your tests passed on CircleCI!
Details
ci/circleci: tsan Your tests passed on CircleCI!
Details

@taiki45 taiki45 deleted the taiki45:default-stats-tag branch Jan 22, 2018

@taiki45

This comment has been minimized.

Member

taiki45 commented Jan 22, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment