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

Introduce Oldtimer (with InfluxDB sink support) #1172

Merged

mwielgus merged 2 commits into kubernetes-retired:master from DirectXMan12:feature/oldtimer

Jul 14, 2016

Contributor

DirectXMan12 commented May 23, 2016 •

edited

Loading

This PR introduces Oldtimer, proposed in docs/proposals/old-timer.md.

Oldtimer is accessible from the /api/v1/historical, and mirrors the model API. It can be enabled by passing --historical_source with one of the URIs used in the --sink argument.

Currently this PR supports the InfluxDB sink. Support for the rest of the metrics sinks should be coming along shortly in separate PRs (although if someone more familiar with one of the individual sinks is interested in writing the Oldtimer support for that sink, that would be appreciated).

cc @burmanm @piosz @mwielgus @bryk

k8s-bot commented May 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

6 similar comments

k8s-bot commented May 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented May 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented May 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented May 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented May 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented May 23, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

DirectXMan12 force-pushed the feature/oldtimer branch from f04f936 to 26b7dc5 Compare

May 23, 2016 16:27

Contributor Author

DirectXMan12 commented May 23, 2016

As far as sinks go, this PR includes InfluxDB, and Hawkular will probably be next (IIRC, @burmanm was interested in helping out there). After that, I'm assuming GCM is the next priority. Depending on time constrains, I can then work on OpenTSDB (or Reimann or Elasticsearch, but I haven't investigated those quite yet).

googlebot added the cla: yes label

DirectXMan12 force-pushed the feature/oldtimer branch from 26b7dc5 to 35772d3 Compare

June 3, 2016 14:25

Contributor Author

DirectXMan12 commented Jun 3, 2016

Ok, I've removed the [WIP] tags from this, since it was decided that additional sinks will get their own PRs.

DirectXMan12 changed the title ~~[WIP] Introduce Oldtimer~~ Introduce Oldtimer

DirectXMan12 changed the title ~~Introduce Oldtimer~~ Introduce Oldtimer (with InfluxDB sink support)

burmanm reviewed

View reviewed changes

metrics/core/historical_types.go

+              	// GetAggregation fetches the given aggregations for one or more objects (specified by metricKeys) of
+              	// the same type, within the given time interval, calculated over a series of buckets
+              	GetAggregation(metricName string, aggregations []string, metricKeys []HistoricalKey, start, end time.Time, bucketSize time.Duration) (map[HistoricalKey][]TimestampedAggregationValue, error)

Contributor

burmanm Jun 7, 2016

I'm not fond of the idea of passing []string type here. Couldn't we make it a type that automatically restricts the available values and makes it easier to read?

Now each sink writer has to know that these strings are actually vars from historical_types.go

Considering that every sink writer has to modify them to their internal naming / functions in any case.

Contributor Author

DirectXMan12 Jun 7, 2016

Just a type alias around string? Sure. I often think that Go could use a proper enum type, though...

DirectXMan12 force-pushed the feature/oldtimer branch from 35772d3 to 1ec15f3 Compare

June 8, 2016 19:50

piosz assigned mwielgus

Contributor

piosz commented Jun 14, 2016

Please do not merge this until we will cut release 1.1 branch.

burmanm reviewed

View reviewed changes

metrics/core/historical_types.go

+              	// GetAggregation fetches the given aggregations for one or more objects (specified by metricKeys) of
+              	// the same type, within the given time interval, calculated over a series of buckets
+              	GetAggregation(metricName string, aggregations []AggregationType, metricKeys []HistoricalKey, start, end time.Time, bucketSize time.Duration) (map[HistoricalKey][]TimestampedAggregationValue, error)

Contributor

burmanm Jun 15, 2016

This question applies to all of these methods. Are there parameters which can be assumed to be passed on with invalid value? Such as end time 0 or start time 0 (or both), bucketSize 0? I see there's checks for these in the example influx implementation, but nothing in these interface definitions.

And if there are, how should they be handled? I can make a lot of assumptions (and derive something from the influx example), but it would be nice to have these documented to make it easier to implement proper HistoricalSource.

Contributor Author

DirectXMan12 Jun 17, 2016

I'll add in documentation to the interface, good catch.

Contributor Author

DirectXMan12 Jun 17, 2016

(but yes, they can have zero/empty values, which indicate no bounds for time, and to use only a single bucket for bucket size).

DirectXMan12 force-pushed the feature/oldtimer branch from 1ec15f3 to 25a6cf7 Compare

June 17, 2016 17:46

ncdc commented Jun 24, 2016

What's the status on this? @DirectXMan12 is there more code to do from your end? @mwielgus does your team have any other comments? I see that the release-1.1 branch exists, so how close do you think we are to merging this?

DirectXMan12 force-pushed the feature/oldtimer branch 4 times, most recently from 0d1857a to 28b15ae Compare

June 29, 2016 21:19

k8s-bot commented Jun 29, 2016

Jenkins GCE e2e

Build/test failed for commit 28b15ae.

Build Log

mwielgus reviewed

View reviewed changes

metrics/api/v1/historical_handlers.go

+              			To(metrics.InstrumentRouteFunc("podMetrics", a.podAggregations)).
+              			Doc("Export some pod-level metric aggregations").
+              			Operation("podAggregations").
+              			Param(ws.PathParameter("pod-id", "The id of the pod to lookup").DataType("string")).

Contributor

mwielgus Jul 1, 2016

What exactly is pod id. Please document.

Contributor Author

DirectXMan12 Jul 1, 2016 •

edited

Loading

ack, will do. It's the UID of the pod (as recorded in the pod metadata in Kubernetes). I'll just change the text there to say UID.

mwielgus reviewed

View reviewed changes

metrics/sinks/influxdb/influxdb_historical.go

+              	typeSel := fmt.Sprintf("type = '%s'", key.ObjectType)
+              	switch key.ObjectType {
+              	case core.MetricSetTypeNode:
+              		return fmt.Sprintf("%s AND %s = '%s'", typeSel, core.LabelNodename.Key, key.NodeName)

Contributor

mwielgus Jul 1, 2016

same here, can we escape the parameters?

Contributor Author

DirectXMan12 Jul 1, 2016 •

edited

Loading

For Influx, key values are checked in checkSanitizedKey above, and an error is thrown if anything besides /^[a-zA-Z0-9_.-]+$/ is found.

Each of the sinks is individually responsible (instead of sanitization being at the API layer) since some sink driver will support bound parameters, which are more flexible.

DirectXMan12 force-pushed the feature/oldtimer branch from 28b15ae to 6a7dce2 Compare

July 1, 2016 16:04

k8s-bot commented Jul 1, 2016

Jenkins GCE e2e

Build/test passed for commit 6a7dce2.

Build Log

DirectXMan12 force-pushed the feature/oldtimer branch from 6a7dce2 to 5834108 Compare

July 2, 2016 14:19

k8s-bot commented Jul 2, 2016

Jenkins GCE e2e

Build/test failed for commit 5834108.

Build Log

burmanm mentioned this pull request

Hawkular Oldtimer support #1209

Closed

DirectXMan12 force-pushed the feature/oldtimer branch from 5834108 to e84ab8f Compare

July 6, 2016 19:16

Contributor Author

DirectXMan12 commented Jul 6, 2016

Ok, I've added tests for the model handlers.

k8s-bot commented Jul 6, 2016

Jenkins GCE e2e

Build/test passed for commit e84ab8f.

Build Log

Contributor

mwielgus commented Jul 7, 2016

Tests fail

Contributor Author

DirectXMan12 commented Jul 7, 2016

Oddly, it passes locally (go version go1.6.1 linux/amd64). Looks like timezones in the timestamps in the test are changing across serialization and deserialization.

Contributor

mwielgus commented Jul 12, 2016

Can you fix it anyhow? I cannot merge a pr that breaks unit test.

Contributor Author

DirectXMan12 commented Jul 13, 2016

@mwielgus yep, just need to figure out what's going on ;-)

DirectXMan12 force-pushed the feature/oldtimer branch from e84ab8f to d642641 Compare

July 13, 2016 19:46

k8s-bot commented Jul 13, 2016

Jenkins GCE e2e

Build/test passed for commit d642641.

Build Log

DirectXMan12 force-pushed the feature/oldtimer branch 2 times, most recently from f355bb9 to b36ca14 Compare

July 13, 2016 20:53

DirectXMan12 added 2 commits

July 13, 2016 16:55


          Oldtimer: Introduce Historical API

e099553

Oldtimer is the Heapster historical metrics access mechanism.  This
commit introduces the the API for Oldtimer, which mirrors the Heapster
model API, with additional paths for retrieving metrics aggregated over
time.

In order for the historical API to function, one specified sink must
support the historical access interface defined in this commit.


          Oldtimer: InfluxDB Support

b36ca14

This commit adds support for historical access (Oldtimer) to the
InfluxDB sink.

Contributor Author

DirectXMan12 commented Jul 13, 2016

Ok, I've pushed one final change, which makes the start time parameter mandatory (and mandatorily non-zero), since different sinks treat a zero time differently, and some sinks have issues with excessively large duration (making an explicit start time mandatory makes sense here). Thanks to @burmanm for pointing out the issue.

Thanks to @ncdc for tracking down the issue with the times. It had to do with Go having a special "Local" timezone internally that was different than the actually local timezone (but "equivalent"), so DeepEqual considered them different (not entirely sure why it worked in east coast time, though).

Contributor Author

DirectXMan12 commented Jul 13, 2016

@mwielgus should be good to go!

k8s-bot commented Jul 13, 2016

Jenkins GCE e2e

Build/test passed for commit f355bb9.

Build Log

k8s-bot commented Jul 13, 2016

Jenkins GCE e2e

Build/test passed for commit b36ca14.

Build Log

Contributor

mwielgus commented Jul 14, 2016

LGTM

mwielgus added the lgtm label

mwielgus merged commit 0e1ec20 into kubernetes-retired:master

DirectXMan12 deleted the feature/oldtimer branch

July 27, 2016 02:39

cainelli mentioned this pull request

instructions to enable oldtimer API #1577

Closed

cblecker unassigned mwielgus

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lgtm