[Merged by Bors] - Add perf reporting to client Producer by tjtelan · Pull Request #2424 · fluvio-community/fluvio

tjtelan · 2022-06-18T18:19:16Z

Resolves #2423

Added ClientStats and ClientStatsUpdate to fluvio crate and integrated into TopicProducer with latency, throughput, memory and cpu stats. This behavior is configurable through TopicProducerConfig using ClientStatsDataCollect enum.

Added user facing ClientStatsDataPoint for display of stats collection. Used in fluvio-cli.

Exposed functionality in the CLI with --stats, --stats-plus, --stats-path options

e.g. fluvio produce <topic> --stats

--stats
- This configures the producer for collecting data for throughput and latency reporting
--stats-plus
- This spawns a separate thread to poll the system for CPU and memory usage.
--stats-path
- When combined with --stats- or --stats-plus, this will write the datapoints to file in csv format for loading into spreadsheet
--no-stats-bar
- Prevent display and update of stats bar. Useful when in combination with --stats-path
--stats-summary
- Implies --stats. Don't print live stats. Only the summary.

crates/fluvio-cli/src/produce/mod.rs

crates/fluvio-extension-common/Cargo.toml

crates/fluvio/src/producer/mod.rs

crates/fluvio/src/producer/partition_producer.rs

sehz · 2022-06-23T03:10:51Z

rebase

sehz

few more comments

crates/fluvio/src/producer/partition_producer.rs

crates/fluvio/src/producer/mod.rs

crates/fluvio-dataplane-protocol/src/batch.rs

crates/fluvio/src/producer/partition_producer.rs

crates/fluvio/src/stats.rs

sehz

Overall looks very good. Just a couple of nits left.

crates/fluvio-dataplane-protocol/src/batch.rs

sehz · 2022-06-25T03:54:15Z

Also, need CLI Test to see if the stat is correct.

sehz · 2022-06-28T19:47:45Z

rebase

crates/fluvio/src/producer/partition_producer.rs

crates/fluvio/src/stats/data_point.rs

sehz

Looks couple. Couple of comments.

crates/fluvio-dataplane-protocol/src/batch.rs

crates/fluvio/Cargo.toml

crates/fluvio/src/producer/partition_producer.rs

Still pretty raw. Not able to get batch size yet

Moved record update further up the callstack Figured out how to get compressed size Wired up consumer to print stats Nothing updated yet, but good start Share stats in multipart consumer stream Created a new PartitionConsumer constructor Naming is hard clean up producer measurements

Still need to adjust, but good start Doing some cleanup

Using the largest reasonable units for quantity Handle some wasm32 compiler errors Reverting changes made to consumer for PR

Finish recording details that were unimplemented Move reporting into cli Optimize some unnecessary destructures Bump crate versions Add to changelog Trying to remove RwLock dependency for stats Addressing some PR feedback

Updating the ArcSwap is a little complicated Address some PR feedback Address PR feedback

Fixing more batch len during conversion Add new methods for getting batch length Hopefully clear up stats collect process

Reorg stats module to be easier to read It compiles, but not yet tested

A little bit of cleanup too

Replaced timestamp using chrono with std::instant Fixed csv writing Fixed interactive stats window

crates/fluvio/Cargo.toml

Respect when data is compressed

sehz

LGTM. Nice work

sehz · 2022-07-09T18:29:53Z

bors r+

Resolves #2423 Added `ClientStats` and `ClientStatsUpdate` to `fluvio` crate and integrated into `TopicProducer` with latency, throughput, memory and cpu stats. This behavior is configurable through `TopicProducerConfig` using `ClientStatsDataCollect` enum. Added user facing `ClientStatsDataPoint` for display of stats collection. Used in fluvio-cli. Exposed functionality in the CLI with `--stats`, `--stats-plus`, `--stats-path` options e.g. `fluvio produce <topic> --stats` * `--stats` * This configures the producer for collecting data for throughput and latency reporting * `--stats-plus` * This spawns a separate thread to poll the system for CPU and memory usage. * `--stats-path` * When combined with `--stats`- or `--stats-plus`, this will write the datapoints to file in csv format for loading into spreadsheet * `--no-stats-bar` * Prevent display and update of stats bar. Useful when in combination with `--stats-path` * `--stats-summary` * Implies `--stats`. Don't print live stats. Only the summary. Co-authored-by: T.J. Telan <t.telan@gmail.com>

bors · 2022-07-09T18:54:46Z

Build failed:

Done

sehz · 2022-07-09T18:57:51Z

armv7 build failed: https://github.com/infinyon/fluvio/runs/7265516240?check_suite_focus=true

sehz · 2022-07-09T18:58:29Z

disable stats for armv7? This probably can be done thru feature flag

Build verified on local $ cross build --bin fluvio --release --target armv7-unknown-linux-gnueabihf Compiling fluvio v0.13.0 (/project/crates/fluvio) Compiling fluvio-extension-common v0.8.0 (/project/crates/fluvio-extension-common) Compiling fluvio-cluster v0.0.0 (/project/crates/fluvio-cluster) Compiling fluvio-cli v0.0.0 (/project/crates/fluvio-cli) Finished release [optimized] target(s) in 1m 01s

tjtelan · 2022-07-09T22:51:06Z

disable stats for armv7? This probably can be done thru feature flag

I fixed this by having 32 bit arm use the same types as wasm32. Builds for me now.

$ cross build --bin fluvio --release --target armv7-unknown-linux-gnueabihf
   Compiling fluvio v0.13.0 (/project/crates/fluvio)
   Compiling fluvio-extension-common v0.8.0 (/project/crates/fluvio-extension-common)
   Compiling fluvio-cluster v0.0.0 (/project/crates/fluvio-cluster)
   Compiling fluvio-cli v0.0.0 (/project/crates/fluvio-cli)
    Finished release [optimized] target(s) in 1m 01s

tjtelan · 2022-07-09T22:51:55Z

bors r+

Resolves #2423 Added `ClientStats` and `ClientStatsUpdate` to `fluvio` crate and integrated into `TopicProducer` with latency, throughput, memory and cpu stats. This behavior is configurable through `TopicProducerConfig` using `ClientStatsDataCollect` enum. Added user facing `ClientStatsDataPoint` for display of stats collection. Used in fluvio-cli. Exposed functionality in the CLI with `--stats`, `--stats-plus`, `--stats-path` options e.g. `fluvio produce <topic> --stats` * `--stats` * This configures the producer for collecting data for throughput and latency reporting * `--stats-plus` * This spawns a separate thread to poll the system for CPU and memory usage. * `--stats-path` * When combined with `--stats`- or `--stats-plus`, this will write the datapoints to file in csv format for loading into spreadsheet * `--no-stats-bar` * Prevent display and update of stats bar. Useful when in combination with `--stats-path` * `--stats-summary` * Implies `--stats`. Don't print live stats. Only the summary. Co-authored-by: T.J. Telan <t.telan@gmail.com>

bors · 2022-07-09T23:21:49Z

Pull request successfully merged into master.

Build succeeded:

Done

Fixing a small display bug from ##2424

tjtelan marked this pull request as ready for review June 22, 2022 04:56

tjtelan requested a review from sehz June 22, 2022 04:56

sehz suggested changes Jun 22, 2022

View reviewed changes

tjtelan requested a review from sehz June 23, 2022 02:22

sehz suggested changes Jun 23, 2022

View reviewed changes

tjtelan requested a review from sehz June 23, 2022 20:26

sehz suggested changes Jun 23, 2022

View reviewed changes

crates/fluvio-dataplane-protocol/src/batch.rs Outdated Show resolved Hide resolved

crates/fluvio/src/producer/partition_producer.rs Outdated Show resolved Hide resolved

crates/fluvio/src/stats.rs Outdated Show resolved Hide resolved

tjtelan requested a review from sehz June 25, 2022 02:49

sehz suggested changes Jun 25, 2022

View reviewed changes

crates/fluvio-dataplane-protocol/src/batch.rs Show resolved Hide resolved

crates/fluvio-dataplane-protocol/src/batch.rs Show resolved Hide resolved

digikata mentioned this pull request Jun 28, 2022

[Bug]: Producer failed to retrieve record metadata when sending more than one batch in a request #2443

Closed

morenol reviewed Jun 29, 2022

View reviewed changes

crates/fluvio/src/producer/partition_producer.rs Outdated Show resolved Hide resolved

morenol reviewed Jun 29, 2022

View reviewed changes

crates/fluvio/src/producer/partition_producer.rs Outdated Show resolved Hide resolved

morenol reviewed Jun 29, 2022

View reviewed changes

crates/fluvio/src/producer/partition_producer.rs Outdated Show resolved Hide resolved

digikata reviewed Jun 30, 2022

View reviewed changes

crates/fluvio/src/stats/data_point.rs Outdated Show resolved Hide resolved

digikata reviewed Jun 30, 2022

View reviewed changes

crates/fluvio/src/stats/data_point.rs Outdated Show resolved Hide resolved

tjtelan requested a review from sehz July 7, 2022 05:13

sehz suggested changes Jul 7, 2022

View reviewed changes

crates/fluvio-dataplane-protocol/src/batch.rs Show resolved Hide resolved

crates/fluvio/Cargo.toml Outdated Show resolved Hide resolved

galibey reviewed Jul 7, 2022

View reviewed changes

crates/fluvio/src/producer/partition_producer.rs Outdated Show resolved Hide resolved

Sehyo Chang and others added 9 commits July 7, 2022 21:08

update third party dep (#2462)

29058ab

producer working

401d961

Still pretty raw. Not able to get batch size yet

ClientStats Display with units

a62cb97

Still need to adjust, but good start Doing some cleanup

Update printed stats display

908df2e

Using the largest reasonable units for quantity Handle some wasm32 compiler errors Reverting changes made to consumer for PR

Producer stats collection now opt-in

b3b11b0

Finish recording details that were unimplemented Move reporting into cli Optimize some unnecessary destructures Bump crate versions Add to changelog Trying to remove RwLock dependency for stats Addressing some PR feedback

Standardize updating shared stats

4a93d98

Updating the ArcSwap is a little complicated Address some PR feedback Address PR feedback

Translate batch_len value during conversion

87303da

Fixing more batch len during conversion Add new methods for getting batch length Hopefully clear up stats collect process

Replace ArcSwap w/ Atomic

468a7f7

Reorg stats module to be easier to read It compiles, but not yet tested

tjtelan added 7 commits July 7, 2022 21:09

Add test to validate dataframe convert

9914330

A little bit of cleanup too

Add stats test to fluvio-test

fd287a2

Add test to CI

f870eac

Fix wasm build

79b1f16

Update CI

73fe7d8

more ci tuning

153a733

PR feedback

19993c4

Replaced timestamp using chrono with std::instant Fixed csv writing Fixed interactive stats window

tjtelan requested a review from sehz July 8, 2022 04:42

sehz suggested changes Jul 8, 2022

View reviewed changes

crates/fluvio/Cargo.toml Outdated Show resolved Hide resolved

tjtelan added 4 commits July 8, 2022 18:45

Add stats feature flag

9903833

Correct Batch<RawRecords> batch len convert

2fd1629

Respect when data is compressed

Clean up some feature imports

0cb7635

Add stats feature to fluvio

da17eba

tjtelan requested a review from sehz July 9, 2022 05:43

add feature gate to producer CLI options

62b0e9c

sehz approved these changes Jul 9, 2022

View reviewed changes

bors bot changed the title ~~Add perf reporting to client Producer~~ [Merged by Bors] - Add perf reporting to client Producer Jul 9, 2022

bors bot closed this Jul 9, 2022

tjtelan mentioned this pull request Jul 13, 2022

[Merged by Bors] - Remove print that duplicates summary #2479

Closed

bors bot pushed a commit that referenced this pull request Jul 13, 2022

Remove print that duplicates summary (#2479)

e9cf0a7

Fixing a small display bug from ##2424

Conversation

tjtelan commented Jun 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sehz commented Jun 23, 2022

Uh oh!

sehz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sehz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sehz commented Jun 25, 2022

Uh oh!

sehz commented Jun 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sehz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sehz left a comment

Choose a reason for hiding this comment

Uh oh!

sehz commented Jul 9, 2022

Uh oh!

bors bot commented Jul 9, 2022

Uh oh!

sehz commented Jul 9, 2022

Uh oh!

sehz commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tjtelan commented Jul 9, 2022

Uh oh!

tjtelan commented Jul 9, 2022

Uh oh!

bors bot commented Jul 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tjtelan commented Jun 18, 2022 •

edited

Loading

sehz commented Jul 9, 2022 •

edited

Loading