Skip to content
This repository has been archived by the owner on Dec 1, 2018. It is now read-only.

Switch Stackdriver sink to grpc-based Stackdriver API #1906

Merged
merged 2 commits into from
Dec 11, 2017
Merged

Switch Stackdriver sink to grpc-based Stackdriver API #1906

merged 2 commits into from
Dec 11, 2017

Conversation

kawych
Copy link
Contributor

@kawych kawych commented Dec 7, 2017

This PR together with #1907 lowers Heapster maximum resource usage by 60% CPU and 25% memory (tested over 2 hours on GCE, 100-node cluster with 3k running user pods).

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 7, 2017
@kawych kawych changed the title [WIP] Switch Stackdriver sink to grpc-based Stackdriver API Switch Stackdriver sink to grpc-based Stackdriver API Dec 7, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2017
@kawych kawych changed the title Switch Stackdriver sink to grpc-based Stackdriver API [WIP] Switch Stackdriver sink to grpc-based Stackdriver API Dec 7, 2017
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 7, 2017
@piosz piosz changed the title [WIP] Switch Stackdriver sink to grpc-based Stackdriver API Switch Stackdriver sink to grpc-based Stackdriver API Dec 8, 2017
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2017
@kawych
Copy link
Contributor Author

kawych commented Dec 8, 2017

@pongad
Can you take a look?

@kawych
Copy link
Contributor Author

kawych commented Dec 8, 2017

/retest

return &sd_api.TimeSeries{
Metric: &sd_api.Metric{
func createTimeSeries(resource string, resourceLabels map[string]string, metadata *metricMetadata, point *monitoringpb.Point) *monitoringpb.TimeSeries {
//monitoringpb.

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@BenTheElder
Copy link

/retest

Copy link

@pongad pongad left a comment

Choose a reason for hiding this comment

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

A couple of nits, but I think it generally looks good.

"google.golang.org/api/googleapi"
sd_api "google.golang.org/api/monitoring/v3"
google_api5 "google.golang.org/genproto/googleapis/api/metric"
google_api4 "google.golang.org/genproto/googleapis/api/monitoredres"
Copy link

Choose a reason for hiding this comment

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

Maybe rename these imports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

timestamp time.Time, labels map[string]string, collectionStartTime time.Time, entityCreateTime time.Time) []*sd_api.TimeSeries {
timeseries := make([]*sd_api.TimeSeries, 0)
timestamp time.Time, labels map[string]string, collectionStartTime time.Time, entityCreateTime time.Time) []*monitoringpb.TimeSeries {
timeseries := make([]*monitoringpb.TimeSeries, 0)
Copy link

Choose a reason for hiding this comment

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

var timeseries []*monitoringpb.TimeSeries? That way empty arrays don't allocate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -268,7 +270,7 @@ func (sink *StackdriverSink) requestSender(reqQueue chan *sd_api.CreateTimeSerie
}
Copy link

Choose a reason for hiding this comment

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

Isn't this loop just

for req := range reqQueue {sink.sendOneRequest(req)}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

startTime := time.Now()
empty, err := sink.stackdriverClient.Projects.TimeSeries.Create(fullProjectName(sink.project), req).Do()
err := sink.stackdriverClient.CreateTimeSeries(context.Background(), req, gax.WithGRPCOptions())
Copy link

Choose a reason for hiding this comment

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

No need to write gax.WithGRPCOptions() if you don't pass options.
This function is used for passing more options to gRPC layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return &sd_api.CreateTimeSeriesRequest{TimeSeries: make([]*sd_api.TimeSeries, 0)}
func getReq(project string) *monitoringpb.CreateTimeSeriesRequest {
return &monitoringpb.CreateTimeSeriesRequest{
TimeSeries: make([]*monitoringpb.TimeSeries, 0),
Copy link

Choose a reason for hiding this comment

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

nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return &monitoringpb.Point{
Interval: &monitoringpb.TimeInterval{
EndTime: &google_protobuf2.Timestamp{Seconds: endTime.Unix(), Nanos: int32(endTime.Nanosecond())},
StartTime: &google_protobuf2.Timestamp{Seconds: startTime.Unix(), Nanos: int32(startTime.Nanosecond())},
Copy link

Choose a reason for hiding this comment

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

Consider using this

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 think it actually returns different type.

Copy link
Contributor

@piosz piosz left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2017
@kawych kawych merged commit 729ea82 into kubernetes-retired:master Dec 11, 2017
kawych added a commit that referenced this pull request Dec 11, 2017
Cherry-pick changes from PR #1906 to 1.5 release branch
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants