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.

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

Chain stream instead of merge #76

Merged
merged 5 commits into from
Aug 21, 2024
Merged

Chain stream instead of merge #76

merged 5 commits into from
Aug 21, 2024

Conversation

RDruon
Copy link
Contributor

@RDruon RDruon commented Aug 20, 2024

Chain stream instead of merging them.

Merge will interleave stream, that's not something we want since it will break prometheus format. See merge documentation at https://docs.rs/tokio-stream/latest/tokio_stream/trait.StreamExt.html#method.merge

Also add a newline to make sure we are printing stats with a separating empty line

From (mind the {operation="open",component="mdt",target="es01a-MDT0000",jobid="node_exporter:989:es-2-virt3-co"}):

[root@node ~]# curl http://localhost:32221/metrics
....
lustre_stats_total{component="mdt",operation="sync",target="es02a-MDT0000"} 6
lustre_stats_total{component="mdt",operation="samedir_rename",target="es02a-MDT0000"} 2
lustre_stats_total{component="mdt",operation="parallel_rename_file",target="es02a-MDT0000"} 2
{operation="open",component="mdt",target="es01a-MDT0000",jobid="node_exporter:989:es-2-virt3-co"} 0
lustre_job_stats_total{operation="close",component="mdt",target="es01a-MDT0000",jobid="node_exporter:989:es-2-virt3-co"} 0
lustre_job_stats_total{operation="mknod",component="mdt",target="es01a-MDT0000",jobid="node_exporter:989:es-2-virt3-co"} 0
....

To:

[root@node ~]# curl http://localhost:32221/metrics
....
lustre_stats_total{component="mdt",operation="sync",target="es02a-MDT0000"} 6
lustre_stats_total{component="mdt",operation="samedir_rename",target="es02a-MDT0000"} 2
lustre_stats_total{component="mdt",operation="parallel_rename_file",target="es02a-MDT0000"} 2

lustre_job_stats_total{operation="open",component="mdt",target="es01a-MDT0000",jobid="node_exporter:989:es-2-virt3-co"} 0
lustre_job_stats_total{operation="close",component="mdt",target="es01a-MDT0000",jobid="node_exporter:989:es-2-virt3-co"} 0
lustre_job_stats_total{operation="mknod",component="mdt",target="es01a-MDT0000",jobid="node_exporter:989:es-2-virt3-co"} 0
...

I haven't been able to reproduce it as part of unit test

@RDruon RDruon added the bug Something isn't working label Aug 20, 2024
@RDruon RDruon self-assigned this Aug 20, 2024
@RDruon RDruon requested a review from jgrund as a code owner August 20, 2024 08:43
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 91.71%. Comparing base (34615b1) to head (fd007df).
Report is 1 commits behind head on main.

Files Patch % Lines
lustrefs-exporter/src/main.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #76      +/-   ##
==========================================
+ Coverage   91.68%   91.71%   +0.03%     
==========================================
  Files          41       41              
  Lines        5207     5228      +21     
  Branches     5207     5228      +21     
==========================================
+ Hits         4774     4795      +21     
  Misses        397      397              
  Partials       36       36              
Flag Coverage Δ
2_14_0_ddn133 35.26% <ø> (ø)
2_14_0_ddn145 36.01% <ø> (ø)
all-tests 91.71% <96.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Benchmark for c1e8a73

Click to view benchmark
Test Base PR %
jobstats 100 4.2±0.60ms 4.4±0.73ms +4.76%
jobstats 1000 40.3±5.02ms 43.7±6.71ms +8.44%

Copy link

Benchmark for 113bc57

Click to view benchmark
Test Base PR %
jobstats 100 4.0±0.62ms 4.3±0.69ms +7.50%
jobstats 1000 38.6±5.08ms 44.6±6.92ms +15.54%

Copy link

Benchmark for 7b4e669

Click to view benchmark
Test Base PR %
jobstats 100 4.1±0.62ms 4.4±0.69ms +7.32%
jobstats 1000 41.8±5.85ms 42.1±6.91ms +0.72%

@RDruon RDruon requested a review from breuhan August 20, 2024 13:29
breuhan
breuhan previously approved these changes Aug 20, 2024
@RDruon RDruon marked this pull request as draft August 20, 2024 13:56
@RDruon RDruon changed the title Add a newline to avoid printing jobstats at same level as previous stats Chain stream instead of merge Aug 20, 2024
@RDruon RDruon marked this pull request as ready for review August 20, 2024 14:19
@RDruon RDruon requested a review from breuhan August 20, 2024 14:19
Copy link

Benchmark for ac5764f

Click to view benchmark
Test Base PR %
jobstats 100 4.0±0.56ms 4.5±0.78ms +12.50%
jobstats 1000 39.6±5.42ms 41.3±6.47ms +4.29%

@jgrund jgrund merged commit fbb9f54 into main Aug 21, 2024
14 checks passed
@jgrund jgrund deleted the rdruon/newline branch August 21, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants