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

Feature/add timeseries prediction crd #2

Merged

Conversation

kitianFresh
Copy link
Contributor

No description provided.

@kitianFresh kitianFresh force-pushed the feature/add-timeseries-prediction-crd branch from ea40cbf to 8d77b70 Compare November 23, 2021 12:50
// MetricName is the name of your metric, such as cpu, mem, disk, network, or something else.
MetricName string `json:"metricName,omitempty"`
// MetricSource is the source of your metric, only supports prometheus now.
MetricSource string `json:"metricSource,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

What is MetricSource for?

Copy link
Contributor

Choose a reason for hiding this comment

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

Where is labels?

@@ -1,6 +1,7 @@
package v1alpha1

import (
"fmt"
Copy link
Contributor

Choose a reason for hiding this comment

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

is go format triggerred? runtime and non-runtime import should be separated

@kitianFresh kitianFresh force-pushed the feature/add-timeseries-prediction-crd branch from 8d77b70 to 64c1ba3 Compare November 29, 2021 03:05
@kitianFresh kitianFresh force-pushed the feature/add-timeseries-prediction-crd branch from 64c1ba3 to 5120290 Compare November 29, 2021 03:36

// Sample pairs a Value with a Timestamp.
type Sample struct {
Value string `json:"value,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a computable type for Value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

this is a nice definition, but i'm not sure it can be used in CRD fields, can we use Quantity here ?
WDYT @yufeiyu @mfanjie

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resource quantity in k8s do not supports float number, we can use string temporarily. later we can implements a type Value like open telemetry in our api not apis crd. because it has some methods definition

// Then next RefreshFrequencySeconds it will do prediction again and update the status of the crd.
// RefreshFrequencySeconds must less than PredictionCycleSeconds. typically it is 1h, 30m, half of your PredictionCycleSeconds.
// Do not set it too small, such as 1s, 1min, 5min, because the predicted data may has no changes in short periods.
RefreshFrequencySeconds int32 `json:"refreshFrequencySeconds,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I can't remember which case should set RefreshFrequencySeconds, if no can we just merge it with predictionCycleSeconds into one field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean one field struct which includes the two? or just one predictionCycleSeconds? it the latter, then the status update is driven by the algorithm data stream and algorithm data source update interval. only the algorithm data updated, predicted data in the prediction crd status updated. the latter we do not expose two much internal behaviors to users

}

// PrometheusQueryExpression
type PrometheusQueryExpression struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest using QueryExpression

}

// CloudMonitorQueryExpression
type CloudMonitorQueryExpression struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

MetricSelector?

@qmhu
Copy link
Member

qmhu commented Nov 29, 2021

/LGTM

@kitianFresh kitianFresh force-pushed the feature/add-timeseries-prediction-crd branch 2 times, most recently from c5f56fb to 99a4108 Compare November 29, 2021 08:55
@yufeiyu
Copy link
Contributor

yufeiyu commented Nov 29, 2021

/lgtm

@kitianFresh kitianFresh merged commit 00d4146 into gocrane:main Nov 29, 2021
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.

None yet

4 participants