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

Allow to limit labels during query via the new SELECTED_LABELS argument #762

Merged
merged 15 commits into from Jul 12, 2021

Conversation

danni-m
Copy link
Collaborator

@danni-m danni-m commented Jun 22, 2021

No description provided.

@lgtm-com
Copy link

lgtm-com bot commented Jun 22, 2021

This pull request introduces 2 alerts when merging ec1568f into 0eb7248 - view on LGTM.com

new alerts:

  • 2 for Missing return statement

@lgtm-com
Copy link

lgtm-com bot commented Jun 23, 2021

This pull request introduces 2 alerts when merging 8b9dcc4 into 1875123 - view on LGTM.com

new alerts:

  • 2 for Missing return statement

@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #762 (01061af) into master (1ea2113) will increase coverage by 0.24%.
The diff coverage is 97.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #762      +/-   ##
==========================================
+ Coverage   89.10%   89.34%   +0.24%     
==========================================
  Files          23       23              
  Lines        3956     4093     +137     
==========================================
+ Hits         3525     3657     +132     
- Misses        431      436       +5     
Impacted Files Coverage Δ
src/reply.c 92.95% <83.33%> (-7.05%) ⬇️
src/gears_commands.c 90.71% <100.00%> (+0.20%) ⬆️
src/gears_integration.c 91.30% <100.00%> (+1.34%) ⬆️
src/module.c 93.33% <100.00%> (+0.02%) ⬆️
src/query_language.c 96.90% <100.00%> (+0.62%) ⬆️
src/redisgears.h 42.82% <100.00%> (+1.40%) ⬆️
src/resultset.c 98.44% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ea2113...01061af. Read the comment docs.

@filipecosta90
Copy link
Contributor

@danni-m @gkorland LVGTM with regards to the overall impact on lastpoint query. Nice work Danni!

Using standalone redis setup:
Overall impact on standalone numbers:

  • lastpoint: from 245.87 queries/sec (p50=85.38 ms) to 566.34 queries/sec (p50=34.49 ms)

lastpoint query WITHLABELS

Run complete after 10000 queries with 20 workers (Overall query rate 245.87 queries/sec):
RedisTimeSeries last row per host:
min:    18.26ms, med:    85.38ms, mean:    81.26ms, max:  135.92ms, stddev:    16.79ms, sum: 812.6sec, count: 10000
all queries                      :
min:    18.26ms, med:    85.38ms, mean:    81.26ms, max:  135.92ms, stddev:    16.79ms, sum: 812.6sec, count: 10000

lastpoint query SELECTED_LABELS

Run complete after 10000 queries with 20 workers (Overall query rate 566.34 queries/sec):
RedisTimeSeries last row per host:
min:     5.37ms, med:    34.49ms, mean:    35.27ms, max:   67.07ms, stddev:     7.81ms, sum: 352.7sec, count: 10000
all queries                      :
min:     5.37ms, med:    34.49ms, mean:    35.27ms, max:   67.07ms, stddev:     7.81ms, sum: 352.7sec, count: 10000

@filipecosta90 filipecosta90 added performance Performance related Issue/PR requires-doc-PR PR requires further documentation labels Jul 6, 2021
@filipecosta90 filipecosta90 added this to In progress in 1.6 via automation Jul 6, 2021
filipecosta90
filipecosta90 previously approved these changes Jul 6, 2021
1.6 automation moved this from In progress to Reviewer approved Jul 6, 2021
@filipecosta90 filipecosta90 changed the title allow to limit labels during query Allow to limit labels during query via the new SELECTED_LABELS argument Jul 6, 2021
1.6 automation moved this from Reviewer approved to Review in progress Jul 7, 2021
@filipecosta90 filipecosta90 self-requested a review July 7, 2021 16:14
filipecosta90
filipecosta90 previously approved these changes Jul 7, 2021
1.6 automation moved this from Review in progress to Reviewer approved Jul 7, 2021
1.6 automation moved this from Reviewer approved to Review in progress Jul 12, 2021
@filipecosta90 filipecosta90 self-requested a review July 12, 2021 15:28
1.6 automation moved this from Review in progress to Reviewer approved Jul 12, 2021
@danni-m danni-m merged commit 5b30b1d into master Jul 12, 2021
@danni-m danni-m deleted the limit_labels_query branch July 12, 2021 16:51
1.6 automation moved this from Reviewer approved to Done Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance related Issue/PR requires-doc-PR PR requires further documentation
Projects
1.6
  
Done
Development

Successfully merging this pull request may close these issues.

Performance last-point: ReplyWithSeriesLabels takes 61% of the on-cpu time of TSDB_mget
2 participants