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

Dynamic series adjustment #64

Merged
merged 15 commits into from
Jul 24, 2024

Conversation

yomek33
Copy link
Contributor

@yomek33 yomek33 commented Jul 15, 2024

Add two new operation modes to the RunMetrics function: gradual-change and double-halve:

double-halve

usage
./avalanche --operation-mode=double-halve --series-change-interval=30 --series-count=20

This mode alternately doubles and halves the series count at regular intervals.
The series count is doubled on one tick and halved on the next, ensuring it never drops below 1.

gradual-change

usage
./avalanche --operation-mode=gradual-change --series-change-interval=30 --series-change-rate=10 series-count=20

This mode gradually changes the series count by a fixed rate at regular intervals
The series count is incremented by seriesChangeRate on each tick, ensuring it never drops below 1.

Tests

  • Added unit tests to cover the new double-halve and gradual-change modes.
  • Verified that the series count adjusts correctly and does not fall below 1.

@yomek33 yomek33 force-pushed the dynamic-series-adjustment branch from 7e5e09e to 519f6be Compare July 15, 2024 22:07
Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few minor implementation comments.

Looks like the tests are failing, there's a make target make test which should run them the same way as Circle CI is where they're failing.

cmd/avalanche.go Outdated
labelInterval = kingpin.Flag("series-interval", "Change series_id label values every {interval} seconds.").Default("60").Int()
metricInterval = kingpin.Flag("metric-interval", "Change __name__ label values every {interval} seconds.").Default("120").Int()
seriesChangeInterval = kingpin.Flag("series-change-interval", "Change the number of series every {interval} seconds.").Default("10").Int()
operationMode = kingpin.Flag("operation-mode", "Mode of operation: 'gradual-change', 'series-spike', 'double-halve'").Default("default").String()
Copy link
Contributor

Choose a reason for hiding this comment

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

series-spike doesn't look like it exists yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add spike mode in next PR! So, I deleted this part of text in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 sounds good

metrics/serve.go Outdated
Comment on lines 188 to 182
case "gradual-change":
go func() {
for tick := range changeSeriesTick.C {
fmt.Printf("%v: Adjusting series count. New count: %d\n", tick, seriesCount)
metricsMux.Lock()
seriesCount += seriesChangeRate
if seriesCount < 1 {
seriesCount = 1
}
unregisterMetrics()
registerMetrics(metricCount, metricLength, metricCycle, labelKeys)
cycleValues(labelKeys, labelValues, seriesCount, seriesCycle)
metricsMux.Unlock()

select {
case updateNotify <- struct{}{}:
default:
}
}
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

one change I'd like to see to this functionality; it would be nice if we gradually increased up to some maximum, and then gradually decreased back down to the starting value (and repeat)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the changes!

metrics/serve.go Outdated
Comment on lines 208 to 259
default:
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the above go routines, before your switch statement, should maybe be the default case?

Otherwise we have both those AND the operation mode we've decided on both modifying the # of metrics and series.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed🙆‍♀️

metrics/serve_test.go Outdated Show resolved Hide resolved
cmd/avalanche.go Outdated
metricCount = kingpin.Flag("metric-count", "Number of metrics to serve.").Default("500").Int()
labelCount = kingpin.Flag("label-count", "Number of labels per-metric.").Default("10").Int()
seriesCount = kingpin.Flag("series-count", "Number of series per-metric.").Default("10").Int()
seriesChangeRate = kingpin.Flag("series-change-rate", "The rate at which the number of active series changes over time. Positive value increases the series, negative value decreases the series.").Default("10").Int()
Copy link
Contributor

Choose a reason for hiding this comment

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

for the flags that only apply to specific operation modes, can we specify which they work for in the help text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some text for that.
And I also tried to add some details for usage of modes like this🤔
image

@yomek33 yomek33 force-pushed the dynamic-series-adjustment branch 3 times, most recently from 8acfd37 to aaf4804 Compare July 17, 2024 09:59
@yomek33 yomek33 marked this pull request as ready for review July 17, 2024 10:01
@yomek33
Copy link
Contributor Author

yomek33 commented Jul 17, 2024

Additionally, I made the following changes:

  • Added a license to the project. This also resolved the CI issues. I set the license year to 2022 to match the other files, but please let me know if it should be updated to 2024.
  • Modified the logic for each Mode. Now, the SeriesCount is adjusted according to the Mode, and the correct SeriesCount is used where necessary.

Please review the updated PR and let me know if you have any further suggestions or concerns.
Thank you!

Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

we could have another PR to move the new operation modes to a separate sub command: https://pkg.go.dev/github.com/alecthomas/kingpin#readme-sub-commands

a few more comments about organization/structure of the code

go.mod Outdated Show resolved Hide resolved
cmd/avalanche.go Outdated Show resolved Hide resolved
cmd/avalanche.go Outdated Show resolved Hide resolved
metrics/serve.go Show resolved Hide resolved
metrics/serve.go Show resolved Hide resolved
@yomek33 yomek33 force-pushed the dynamic-series-adjustment branch from 1c44292 to 28678fb Compare July 18, 2024 13:54
@cstyan
Copy link
Contributor

cstyan commented Jul 19, 2024

If you rebase or merge in master it looks like Ben updated the build dependencies recently, so we don't need the change to upgrade to go1.21 anymore. Also, for the DCO check to pass (here) you need to sign commits with git commit -s.

@yomek33 yomek33 force-pushed the dynamic-series-adjustment branch from 3cc0f5b to 55c0c4a Compare July 19, 2024 13:11
@yomek33
Copy link
Contributor Author

yomek33 commented Jul 19, 2024

marged and I signed

metrics/serve.go Outdated Show resolved Hide resolved
@yomek33 yomek33 force-pushed the dynamic-series-adjustment branch from 692db74 to 8d7c0c5 Compare July 19, 2024 22:51
@yomek33 yomek33 force-pushed the dynamic-series-adjustment branch from 8d7c0c5 to fddb408 Compare July 19, 2024 23:04
yomek33 added 2 commits July 20, 2024 08:13
Signed-off-by: yomek33 <[email protected]>
Signed-off-by: yomek33 <[email protected]>
Copy link
Contributor

@cstyan cstyan left a comment

Choose a reason for hiding this comment

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

I left a few comments for code change suggestions; these simplify the code a bit and also make sure that the two new operation modes have metrics available for scraping immediately. We either need this change or to set currentSeriesCount = seriesCount in each of the two new modes as you've done in the default case.

cmd/avalanche.go Outdated Show resolved Hide resolved
metrics/serve.go Outdated Show resolved Hide resolved
metrics/serve.go Outdated Show resolved Hide resolved
metrics/serve.go Outdated Show resolved Hide resolved
metrics/serve.go Outdated Show resolved Hide resolved
@yomek33 yomek33 force-pushed the dynamic-series-adjustment branch from 77cb088 to bddbb3b Compare July 23, 2024 19:08
metrics/serve.go Outdated Show resolved Hide resolved
Signed-off-by: yomek33 <[email protected]>
@yomek33 yomek33 force-pushed the dynamic-series-adjustment branch from 56328e0 to a41aecf Compare July 24, 2024 16:48
@cstyan
Copy link
Contributor

cstyan commented Jul 24, 2024

@yomek33 good work!

@cstyan cstyan merged commit 9caa882 into prometheus-community:main Jul 24, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants