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

spike mode for dynamic series adjustment #70

Merged
merged 4 commits into from
Aug 22, 2024

Conversation

yomek33
Copy link
Contributor

@yomek33 yomek33 commented Jul 30, 2024

Add spike mode for metrics generation and cycling. Other two modes: #64

spike

usage
./avalanche --series-operation-mode=spike --series-change-interval=180 --series-count=100 --spike-multiplier=1.5
This mode periodically increases the series count by a spike multiplier on one tick and then returns it to the original count on the next tick. This pattern repeats indefinitely.

@yomek33 yomek33 marked this pull request as ready for review July 30, 2024 20:13
@yomek33
Copy link
Contributor Author

yomek33 commented Jul 30, 2024

Currently, the implementation alternates between the original series count and the spiked series count at each seriesChangeInterval. This approach was chosen for its simplicity. However, if we want to have the original series count for the first 3 minutes and then the spiked series count for the next 5 minutes, we could achieve this by adding a new flag. This would allow for more flexible and customized patterns for metrics generation. If this is not a concern, we can proceed with the current implementation.

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.

just some minor comments

and now that I think about it some more, we can probably combine this mode and double-halve, since this is essentially double-halve with a spike-multiplier of 2

metrics/serve.go Outdated
Comment on lines 211 to 214
if inSpike {
inSpike = false
*currentSeriesCount = initialSeriesCount
} else {
inSpike = true
*currentSeriesCount = int(float64(initialSeriesCount) * spikeMultiplier)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you could simplify this by just checking

if *currentSeriescount > initialSeriesCount {
    set to initial series count
    continue
}
multiply by the spike multiplier

Comment on lines +221 to +220
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.

not sure what's happening here, and looking at the other handle functions we're doing the same, what do we need this select and the update notify channel for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the updateNotify channel are used here to:

avalanche/metrics/write.go

Lines 140 to 149 in 4b732e1

select {
case <-c.config.UpdateNotify:
log.Println("updating remote write metrics")
tss, err = collectMetrics()
if err != nil {
merr.Add(err)
}
default:
tss = updateTimetamps(tss)
}

Each time a notification is received, the metrics are updated and logged.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right! We learned something. It looks like updateNotify is only used for the write mode, where avalanche is remote writing somewhere. So this isn't needed for what we're trying to do with prombench, but it is required in general 👍

@cstyan
Copy link
Contributor

cstyan commented Aug 2, 2024

might need to rebase or merge in master, looks like there might be a merge conflict?

@yomek33
Copy link
Contributor Author

yomek33 commented Aug 6, 2024

might need to rebase or merge in master, looks like there might be a merge conflict?

@cstyan I fixed!🙆‍♀️

@yomek33 yomek33 force-pushed the series-spike branch 3 times, most recently from 69d0f7d to 98823c6 Compare August 7, 2024 04:17
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.

just some minor comments, only one thing that should change for now IMO

Comment on lines 201 to 205
assert.Equal(t, expectedCount, currentCount, "Halved series count should be %d but got %d", int(expectedCount), currentCount)
} else {
currentCount := countSeries(t, promRegistry)
expectedCount := initialSeriesCount * spikeMultiplier
assert.Equal(t, int(expectedCount), currentCount, "Doubled series count should be %d but got %d", int(expectedCount), float64(currentCount))
Copy link
Contributor

Choose a reason for hiding this comment

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

we should change the messages here, you could put "multiplied the series count by %d, should be %d but got %d" for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed

@@ -91,7 +99,7 @@ func main() {

stop := make(chan struct{})
defer close(stop)
updateNotify, err := metrics.RunMetrics(*metricCount, *labelCount, *seriesCount, *seriesChangeRate, *maxSeriesCount, *minSeriesCount, *metricLength, *labelLength, *valueInterval, *labelInterval, *metricInterval, *seriesChangeInterval, *seriesOperationMode, *constLabels, stop)
updateNotify, err := metrics.RunMetrics(*metricCount, *labelCount, *seriesCount, *seriesChangeRate, *maxSeriesCount, *minSeriesCount, *metricLength, *labelLength, *valueInterval, *labelInterval, *metricInterval, *seriesChangeInterval, *spikeMultiplier, *seriesOperationMode, *constLabels, stop)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to do it now, but we should look at breaking RunMetrics into separate functions for each of the run modes. The current functions such as this new handleSpikeMode could create the other go routines that we create within RunMetrics currently.

This would mean we don't have to pass so many parameters around across a few functions.

@cstyan cstyan merged commit 3558d56 into prometheus-community:main Aug 22, 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