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: native DogStatsD support #2158

Merged
merged 28 commits into from Dec 11, 2017

Conversation

Projects
None yet
3 participants
@taiki45
Copy link
Member

taiki45 commented Dec 5, 2017

Description:
Add new stats sink which support tagged metrics with DogStatsD format.
This feature is configurable by specifying envoy.dog_statsd in Bootstrap.stats_sinks.
Config and documentation changes are here: envoyproxy/data-plane-api#325.
Original issue is #2024.

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

Testing:
unit test, and manual testing with statsd_exporter which supports DogStatsD extension and Prometheus.

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

Release Notes:
Added, but I'm not sure about format. I referred https://github.com/envoyproxy/data-plane-api/blob/master/docs/root/intro/version_history.rst.

Fixes #2024

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

Review Note:
I'm not a fluent English speaker. Please feel free to point out any bit of difference, thanks.

taiki45 added some commits Nov 30, 2017

stats: native DogStatsD support
Add new stats sink which supports tagged metrics with DogStatsD format.
At the first moment, the sink only supports UDP output.

Tested with stastd-exporter v0.5.0 and Prometheus v2.0.0.

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
stats: point to temporary fork
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
docs: add release note
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

taiki45 added some commits Dec 5, 2017

stats: format with clang-format-5.0
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Fix test class name
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Writer::Writer(Network::Address::InstanceConstSharedPtr address) : Statsd::Writer(address) {}

void Writer::writeCounter(const std::string& name, const std::string& tag_str, uint64_t increment) {
std::string message(fmt::format("envoy.{}:{}|c{}\n", name, increment, tag_str));

This comment has been minimized.

@htuch

htuch Dec 5, 2017

Member

At a high-level, wondering if you could comment on the tradeoff between doing this as a separate sink vs. making this a configurable option on the existing statsd sink? It seems the formats are fairly similar, with regular `statsd a subset. Don't mind a separate sink if it makes more sense.

This comment has been minimized.

@taiki45

taiki45 Dec 5, 2017

Member

Right, it's another option to implement this. In my opinion, pros of a separate sink: dedicated sink confiuration brings future extensibility on configuration around tagging. Then, pros of exteding existing statsd sink: compact code base and configuration.

I think we have to think which is more user-friendly idea not from point of code-base. In my current opinion, since the original issue #2024 starts from a new sink with dedicated configuration, the new sink seems more human natural idea. What do you think?

For code-base or design points, I'm welcome to any advices and will make improvements.

This comment has been minimized.

@mattklein123

mattklein123 Dec 5, 2017

Member

@taiki45 I think having a dedicated configuration makes sense from the user perspective. I think the main question is whether to have a shared base class for the implementation (or just one implementation with some functionality variable built in). I don't really have that strong of an opinion either way as the code is pretty self contained.

This comment has been minimized.

@taiki45

taiki45 Dec 5, 2017

Member

Aha, now I got the question. In this case, for code parspective, I agree to change current implementation because inheritting from concrete class (Envoy::Stats::Statsd::UdpStatsdSink class here) slightly smells bad for me.

I'm thinking about using and extending existing Envoy::Stats::Statsd::UdpStatsdSink instead of having a shared base class. Any advices?

This comment has been minimized.

@mattklein123

mattklein123 Dec 5, 2017

Member

Sure that SGTM.

This comment has been minimized.

@taiki45

taiki45 Dec 5, 2017

Member

Done in 5937ac4.

taiki45 added some commits Dec 5, 2017

Use and extend existing statsd sink class
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Update SHA of data-plane-api
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
@taiki45

This comment has been minimized.

Copy link
Member

taiki45 commented Dec 6, 2017

hmm... continuous-integration/travis-ci seems not working? It's waiting forever...

@mattklein123 mattklein123 reopened this Dec 6, 2017

@mattklein123

This comment has been minimized.

Copy link
Member

mattklein123 commented Dec 6, 2017

@taiki45 sometimes CI is just broken. You need to merge master anyway so just push a new commit and it will probably fix itself.

Merge remote-tracking branch 'origin/master' into dog-statsd
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
@taiki45

This comment has been minimized.

Copy link
Member

taiki45 commented Dec 6, 2017

OK, I've resolved the conflicts and pushed the merge commit.

@htuch

This comment has been minimized.

Copy link
Member

htuch commented Dec 6, 2017

Looks like //test/integration:integration_test is timing out on OS X, but I'd hazard a guess that this is not your PR and is general flake, I will kick it again.

@htuch
Copy link
Member

htuch left a comment

Refactored approach looks good.

if (i == 0) {
tagStr.append(fmt::format("{}:{}", tag.name_, tag.value_));
} else {
tagStr.append(fmt::format(",{}:{}", tag.name_, tag.value_));

This comment has been minimized.

@htuch

This comment has been minimized.

@taiki45

taiki45 Dec 7, 2017

Member

Good. be26f9f

Also, I'm curious about better ways to join std::vector<Tag>& tags with "," into single string, joining each tag with ":". I did some googling but I can't found the way...

return {};
}

std::string tagStr = "|#";

This comment has been minimized.

@htuch

htuch Dec 6, 2017

Member

Snake rather than camel case for locals, i.e. tag_str.

}

const std::string buildTagStr(const std::vector<Tag>& tags) {
if (tags.empty()) {

This comment has been minimized.

@htuch

htuch Dec 6, 2017

Member

Should this be use_tag_ || tags.empty() to preserve existing behavior?

This comment has been minimized.

@taiki45
ThreadLocal::SlotPtr tls_;
Network::Address::InstanceConstSharedPtr server_address_;
bool useTag_;

This comment has been minimized.

@htuch

htuch Dec 6, 2017

Member

Snake rather than camel for member variables, i.e. use_tag_.

This comment has been minimized.

@htuch

htuch Dec 6, 2017

Member

Also, this can be const bool.

ON_CALL(counter, tagExtractedName()).WillByDefault(ReturnRef(name));
ON_CALL(counter, tags()).WillByDefault(ReturnRef(tags));

sink.flushCounter(counter, 1);

This comment has been minimized.

@htuch

htuch Dec 6, 2017

Member

I don't see the actual expected statsd raw output in any of these tests? Shouldn't we have something like this?

I think this is an issue with the existing tests; there's no validation that we're actually building the correct string.

This comment has been minimized.

@taiki45

taiki45 Dec 7, 2017

Member

Yes, agree to add the validation test. I'll try it in this PR.

This comment has been minimized.

@htuch

htuch Dec 7, 2017

Member

Thanks, this would be a nice improvement to the existing tests.

This comment has been minimized.

@taiki45

taiki45 Dec 8, 2017

Member

It was a little bit difficult for me, but I tried with changing Writer's responsibility.
How about this one: e3487df

taiki45 added some commits Dec 7, 2017

Use snake_case for local var name
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Don't build tag string unless using tag
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Use StringUtil::join
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Use snake_case for member variables
Also use `const` for bool.

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

htuch added a commit to envoyproxy/data-plane-api that referenced this pull request Dec 7, 2017

stats: add config and docs for dog statsd sink (#325)
For envoyproxy/envoy#2158.

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
void Writer::writeCounter(const std::string& name, uint64_t increment) {
std::string message(fmt::format("envoy.{}:{}|c", name, increment));
void Writer::writeCounter(const std::string& name, const std::string& tag_str, uint64_t increment) {
std::string message(fmt::format("envoy.{}:{}|c{}\n", name, increment, tag_str));

This comment has been minimized.

@htuch

htuch Dec 7, 2017

Member

Should we avoid the \n for regular statsd? i.e. only add it in buildTagStr() if use_tag_?

This comment has been minimized.

@taiki45

taiki45 Dec 8, 2017

Member

Nice point. Actually we don't need trailing newline, so just dropped it. b664048

for (const Tag& tag : tags) {
if (i == 0) {
std::vector<std::string> v = {tag.name_, tag.value_};
tag_str.append(StringUtil::join(v, ":"));

This comment has been minimized.

@htuch

htuch Dec 7, 2017

Member

The idea with join is you do something like:

std::vector<std::string> tag_strs;
for (const Tag& tag : tags) {
  tag_strs.emplace_back(tag._name_ + ":" + tag._value);
}
return "|#" + StringUtil::join(tag_strs, ",");

This is probably less performant than what you have, but more concise. Since this is only at statsd flush time, it's probably fine; if we hit scaling issues then we can micro-optimize.

This comment has been minimized.

@taiki45

taiki45 Dec 8, 2017

Member

Since this is only at statsd flush time, it's probably fine

Agreed. Refine with 3c38668.

ON_CALL(counter, tagExtractedName()).WillByDefault(ReturnRef(name));
ON_CALL(counter, tags()).WillByDefault(ReturnRef(tags));

sink.flushCounter(counter, 1);

This comment has been minimized.

@htuch

htuch Dec 7, 2017

Member

Thanks, this would be a nice improvement to the existing tests.

taiki45 added some commits Dec 8, 2017

Pin to original data-plane-api
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Omit unnecessary newline character
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Add test to check actual sent values to writer
Remove process of building stats string from Writer and it now only holds
a fd.

Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Use snake_case and const modifier
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Simplify buildTagStr()
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>

taiki45 added some commits Dec 8, 2017

Merge remote-tracking branch 'origin/master' into dog-statsd
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Add a comment
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
@mattklein123
Copy link
Member

mattklein123 left a comment

Generally looks good. A bunch of nits and a little extra test coverage needed. Thank you!

tls_->set([this](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr {
return std::make_shared<Writer>(this->server_address_);
});
}

void UdpStatsdSink::flushCounter(const Counter& counter, uint64_t delta) {
tls_->getTyped<Writer>().writeCounter(counter.name(), delta);
std::string message(

This comment has been minimized.

@mattklein123

mattklein123 Dec 11, 2017

Member

nit: const here and similar places, or just kill local variable and do inline as var not needed.

This comment has been minimized.

@taiki45

taiki45 Dec 11, 2017

Member

Yep, use const d3fb9ef.


const std::string UdpStatsdSink::buildTagStr(const std::vector<Tag>& tags) {
if (!use_tag_ || tags.empty()) {
return {};

This comment has been minimized.

@mattklein123

mattklein123 Dec 11, 2017

Member

nit: I would probably use "" here as that is more idiomatic.

This comment has been minimized.

@taiki45
return {};
}

std::vector<std::string> tag_strs;

This comment has been minimized.

@mattklein123

mattklein123 Dec 11, 2017

Member

nit: s/tag_strs/tag_strings

void writeGauge(const std::string& name, uint64_t value);
void writeTimer(const std::string& name, const std::chrono::milliseconds& ms);
// For testing.
Writer() : fd_(-1){};

This comment has been minimized.

@mattklein123

mattklein123 Dec 11, 2017

Member

nit: del ';' at end of line

UdpStatsdSink(ThreadLocal::SlotAllocator& tls, Network::Address::InstanceConstSharedPtr address,
const bool use_tag);
// For testing.
UdpStatsdSink(ThreadLocal::SlotAllocator& tls, std::shared_ptr<Writer> w, const bool use_tag)

This comment has been minimized.

@mattklein123

mattklein123 Dec 11, 2017

Member

nit: s/w/writer


namespace Envoy {
namespace Stats {
namespace Statsd {

class MockWriter : public Writer {
public:
MOCK_METHOD1(write, void(const std::string& m));

This comment has been minimized.

@mattklein123

mattklein123 Dec 11, 2017

Member

nit: s/m/message

int fd = sink.getFdForTests();
EXPECT_NE(fd, -1);

// Check that fd has not changed.
std::vector<Tag> tags;

This comment has been minimized.

@mattklein123

mattklein123 Dec 11, 2017

Member

I would recommend moving this into the base mock class, and then putting the ON_CALL WillByDefault into the constructor, so that it can be use by others later.

This comment has been minimized.

@taiki45

taiki45 Dec 11, 2017

Member

Cool, how about this one? 85dab7f

ON_CALL(counter, tags()).WillByDefault(ReturnRef(tags));
EXPECT_CALL(*std::dynamic_pointer_cast<NiceMock<MockWriter>>(writer_ptr),
write("envoy.test_counter:1|c"));
;

This comment has been minimized.

@mattklein123

mattklein123 Dec 11, 2017

Member

nit: remove ';' on lines by itself in this file.

namespace Server {
namespace Configuration {

Stats::SinkPtr DogStatsdSinkFactory::createStatsSink(const Protobuf::Message& config,

This comment has been minimized.

@mattklein123

mattklein123 Dec 11, 2017

Member

Please make sure that the actual registration system has coverage. You could either add this to one of the v2 integration test configurations or add dedicated config loading tests here: https://github.com/envoyproxy/envoy/blob/master/test/server/config/stats/config_test.cc. Note that you can check your coverage in CI by opening up the CI coverage build and looking at the build artifacts in CircleCI.

This comment has been minimized.

@taiki45

taiki45 Dec 11, 2017

Member

Done fc4efda

Note that you can check your coverage in CI by opening up the CI coverage build and looking at the build artifacts in CircleCI.

I just didn't know, thanks!

taiki45 added some commits Dec 11, 2017

Use const if available
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Use "" for more readability
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
s/tag_strs/tag_strings/
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Remove unnecessary end of ";"
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
clang-format
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Use more clear name
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Move default method mocks to constructor
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
Tests for DogStatsdSinkFactory
Signed-off-by: Taiki Ono <taiks.4559@gmail.com>
@mattklein123
Copy link
Member

mattklein123 left a comment

LGTM, thanks!

@mattklein123 mattklein123 merged commit 112f6c6 into envoyproxy:master Dec 11, 2017

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: 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
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taiki45

This comment has been minimized.

Copy link
Member

taiki45 commented Dec 12, 2017

Thank you all too!

@taiki45 taiki45 deleted the taiki45:dog-statsd branch Dec 12, 2017

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