Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implemented simple metrics reporter #3066

Merged
merged 9 commits into from
Dec 8, 2021
Merged

Implemented simple metrics reporter #3066

merged 9 commits into from
Dec 8, 2021

Conversation

mmaslankaprv
Copy link
Member

@mmaslankaprv mmaslankaprv commented Nov 24, 2021 •
edited
Loading

Cover letter

Implemented simple metrics reporter.

Fixes: #2707

Release notes

@mmaslankaprv mmaslankaprv changed the title Simple metrics reporter Implemented simple metrics reporter Nov 24, 2021
@mmaslankaprv mmaslankaprv marked this pull request as draft November 24, 2021 11:19
@mmaslankaprv mmaslankaprv marked this pull request as ready for review November 24, 2021 18:47
@mmaslankaprv mmaslankaprv force-pushed the simple-metrics-reporter branch 2 times, most recently from 99ce58d to 79516d4 Compare November 25, 2021 10:16
dotnwat
dotnwat reviewed Dec 1, 2021
View reviewed changes
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

looks awesome. a few minor comments

src/v/cluster/metrics_reporter.cc Show resolved Hide resolved
src/v/cluster/metrics_reporter.cc Outdated Show resolved Hide resolved
src/v/cluster/metrics_reporter.cc Show resolved Hide resolved
src/v/cluster/metrics_reporter.h Outdated
Comment on lines 83 to 84
static constexpr std::string_view host = "m.rp.vectorized.io";
static constexpr std::string_view path = "v2";
Copy link
Member

@dotnwat dotnwat Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

if we use a configuration value for this then it will be straight forward to write a duck tape test. a web server can be spun up in a few lines of python: https://docs.python.org/3/library/http.server.html and we could wrap that in a duck tape background service. then arrange for the configuration to dial home to our wrapper web server.

Copy link
Member Author

Choose a reason for hiding this comment

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

i've made the url configurable, will add ducktape test in follow up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added test for metrics reporter in ducktape

src/v/cluster/metrics_reporter.cc Outdated Show resolved Hide resolved
@dotnwat
Copy link
Member

dotnwat commented Dec 1, 2021

mmaslankaprv requested a review from dotnwat 10 hours ago is ambiguous since it looks like nothing new was pushed. are you saying you don't want to change anything?

@mmaslankaprv mmaslankaprv force-pushed the simple-metrics-reporter branch 2 times, most recently from 9a3336e to 7f4ea57 Compare December 2, 2021 10:18
dotnwat
dotnwat requested changes Dec 3, 2021
View reviewed changes
src/v/cluster/metrics_reporter.cc Outdated Show resolved Hide resolved
jcsp
jcsp reviewed Dec 6, 2021
View reviewed changes
Copy link
Contributor

@jcsp jcsp left a comment

Choose a reason for hiding this comment

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

Sorry I'm a bit late looking at this -- just a couple of comments (also I was going to make the same suggestion as Noah to add a webserver stub in ducktape to test this)

src/v/config/configuration.cc Outdated Show resolved Hide resolved
src/v/config/configuration.cc Show resolved Hide resolved
src/v/cluster/metrics_reporter.cc
reader_cfg.type_filter = model::record_batch_type::raft_configuration;
auto reader = co_await _raft0->make_reader(reader_cfg);

auto batches = co_await model::consume_reader_to_memory(
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling with no_timeout, is it abortable some other way if this is running during shutdown?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is not, but this is going to be really fast since there are only two batches, and they are very small.

dotnwat
dotnwat reviewed Dec 8, 2021
View reviewed changes
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

looks good. nothing major from me, but a couple test suggestions. I think John had a good point about configuration option naming, and he might also want to take a peek at the duck tape tests.

tests/rptest/services/http_server.py Outdated Show resolved Hide resolved
tests/rptest/services/http_server.py Outdated Show resolved Hide resolved
src/v/cluster/metrics_reporter.cc Outdated Show resolved Hide resolved
@mmaslankaprv mmaslankaprv force-pushed the simple-metrics-reporter branch 2 times, most recently from 68393d2 to 7c1aa8e Compare December 8, 2021 11:57
@jcsp
Copy link
Contributor

jcsp commented Dec 8, 2021

New tests LGTM apart from one stray line. Looks like this has a consistent failure in WaitForLocalConsumerTest though, I haven't spotted what part of the test changes might be triggering that :-/

@mmaslankaprv
rpc/transport: silent noisy connection error log
d31e3aa 
When connecting to remote endpoint `rpc::transport::connect` caller
should take care of error logging. Adding warning log entry every time
connection failed may be very noisy in some scenarios f.e. retries.

Signed-off-by: Michal Maslanka <michal@vectorized.io>
@mmaslankaprv
config: added metrics reporter configuration options
70e47b7 
Signed-off-by: Michal Maslanka <michal@vectorized.io>
@mmaslankaprv
c/metrics_reporter: implemented simple periodic metrics reporter
844e956 
Implemented simple service that will gather most important cluster
metrics and send them periodically to given URL address. The metrics are
sent only from the cluster controller and contain information about all
the nodes.

Signed-off-by: Michal Maslanka <michal@vectorized.io>
@mmaslankaprv
c/metrics_reporter: added url parsing tests
3b567b9 
Signed-off-by: Michal Maslanka <michal@vectorized.io>
@mmaslankaprv
controller: wired up metrics_reporter
a7bf273 
Signed-off-by: Michal Maslanka <michal@vectorized.io>
@mmaslankaprv
tests: disable metrics reporter in ducktape tests
caa329f 
Signed-off-by: Michal Maslanka <michal@vectorized.io>
@mmaslankaprv
tests: added http server background service
9af499d 
Added simple http server background service printing each received
request to stdout. Requests can be accessed and verified afterwards.

Signed-off-by: Michal Maslanka <michal@vectorized.io>
@mmaslankaprv
tests: added metrics reporter ducktape test
0f05674 
Signed-off-by: Michal Maslanka <michal@vectorized.io>
@mmaslankaprv
tests/kaf_consumer: add missing follow flag
23b3408 
Added `-f` flag to `kaf` consumer background service. Without the flag
kaf exits immediately when there are no messages in topic

Signed-off-by: Michal Maslanka <michal@vectorized.io>
@mmaslankaprv mmaslankaprv added this to the v21.11.2 milestone Dec 8, 2021
@mmaslankaprv mmaslankaprv merged commit bd6335c into redpanda-data:dev Dec 8, 2021
@twmb
Copy link
Contributor

twmb commented Dec 13, 2021

Does this need docs updates?

@BenPope BenPope mentioned this pull request Dec 29, 2021
Merged
@mmaslankaprv mmaslankaprv mentioned this pull request Feb 3, 2022
Merged
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotnwat dotnwat dotnwat approved these changes

@ivotron ivotron Awaiting requested review from ivotron

@NyaliaLui NyaliaLui Awaiting requested review from NyaliaLui

@VadimPlh VadimPlh Awaiting requested review from VadimPlh

@ztlpn ztlpn Awaiting requested review from ztlpn

@emaxerrno emaxerrno Awaiting requested review from emaxerrno

@jcsp jcsp Awaiting requested review from jcsp

Assignees
No one assigned
Labels
area/redpanda
Projects
None yet
Milestone
v21.11.2
Development

Successfully merging this pull request may close these issues.

Move usage stats reporting into redpanda
4 participants
@mmaslankaprv @dotnwat @jcsp @twmb