Specification for exposing apis to access time-series data #63

anmolbabu · 2016-12-13T20:52:53Z

tendrl-bug-id: #62
Signed-off-by: anmolbabu anmolbudugutta@gmail.com

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

anmolbabu · 2016-12-13T20:54:31Z

@anivargi I have documented from monitoring perspective could you please add specifics from the perspective of tendrl-api

@anivargi @shtripat @nthomas-redhat @r0h4n @brainfunked please review.

TODO: Create issues on related components and link it in spec

shtripat · 2016-12-14T06:31:13Z

specs/time_series_api.adoc

+The above response gives the statistics for last 24 hours corresponding to the
+'resource_name' passed to it. If a time range is passed as optional arguements
+to the above api, then the api responds with the statistics for that time
+range.


Can you add a sample for time range based invocation as well here?

shtripat · 2016-12-14T06:32:15Z

specs/time_series_api.adoc

+=== Developer impact:
+
+This has extensive impact on 2 modules tendrl-api and performance-monitoring
+and hence warrants a very high collaboration between the implementers of the


%s/implementers/owners/g

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

shtripat

LGTM other than two minor comments I have.

shtripat · 2016-12-15T04:01:25Z

specs/time_series_api.adoc

+=== Assignee(s):
+
+
+Who is leading the writing of the code? Or is this a spec where you're


I feel line 253-257 is not required. Its retained from sample I feel. Please remove.

Yes I was adding to the template and while doing so I forgot to remove this :)

r0h4n · 2016-12-15T06:32:19Z

specs/time_series_api.adoc

+        touching any other part of the code.
+    *** The way the plugins work is :
+      **** The interfaces to time-series db are added under the directory
+           tendrl/performance-monitoring/time_series_db/dbplugins of


If these are etcd directories, please use "tendrl/" instead of "tendrl/", Etcd hides directories with a leading "" and we need to use it

No it is location in tendrl/performance-monitoring code-base where the specific time-series db handlers need to be added

In that case, omit the tendrl/performance-monitoring prefix from the directory name, since you've mentioned the repository name at the end of the sentence anyway.

nthomas-redhat · 2016-12-15T07:16:01Z

specs/time_series_api.adoc

+            ***** Since the manager is a singleton, there is only one instance
+                  of it throughout the life of Tendrl/performance-monitoring
+                  application which means all this house-keeping is done only
+                  once during the lifecycle of the application.


Line 58-87, does this implementaion covered as part of this spec?

This spec talks about apis that fetch time series data. So I added more details like how the apis actually fetch the details i.e, the api talks to a manager that takes the responsibility of finding the correct plugins that actually talks to time-series db

nthomas-redhat · 2016-12-15T07:20:02Z

specs/time_series_api.adoc

+]
+----------
+
+The above response gives the statistics for last 24 hours corresponding to the


is this the latest value or 24 hours value?

This is 24 hours value :) I cut short the sample because it was too huge and symbolically added ',.... ' notation

How to get the latest value(single last updated )?

like the time option, there needs to be one more option that internally does the graphite o/p parsing like we used to do before I'll add a sample for that as well

Please add these details to the doc

nthomas-redhat · 2016-12-15T07:20:35Z

specs/time_series_api.adoc

+
+=== Security impact:
+
+The apis exposed by Tendrl/performance-monitoring although are only getters,


Please add the details here

Ok. I'll update this soon. Looking through details on flask docs.

nthomas-redhat · 2016-12-15T07:22:05Z

specs/time_series_api.adoc

+
+==== Tendrl API impact:
+
+The tendrl-api interfaces to the apis mentioned above under the section


I would like see more details on how the tendrl-api to performance-monitoring interaction happens

@anivargi Can you please provide your inputs here

@nthomas-redhat Making call's to the monitoring app via a http client programatically. The API will define its own url's, somewhat similar to what API;s from the monitoring app. We need to take this approach since we need to have one point of interaction.

@anmolbabu @anivargi , Please add these details to the doc

nthomas-redhat · 2016-12-15T07:24:01Z

specs/time_series_api.adoc

+Some of the config options as below need to be made available:
+
+----
+[time_series]


Is this something to be covered as mentioned in - #11

Ok I'll remove it from here. Its anyway covered in #20 in lines 286 to 297

brainfunked

The document needs integration details from @anivargi as has already been pointed out. The point I'd like to be considered when adding these details is that the interface must appear to be uniform. To the client, it should not be apparent that there are in fact two separate APIs being accessed to provide this functionality.

brainfunked · 2016-12-15T12:53:28Z

specs/time_series_api.adoc

+  ** Interface to time-series db is designed in the form of plugins specific to
+     a time-series db.
+    *** This means that to support a new time-series db, all it takes is the
+        addition of its apecic interface as a plugin and doesn't require


specific :)

brainfunked · 2016-12-15T12:54:06Z

specs/time_series_api.adoc

+
+== Proposed change
+
+* Tendrl/performance-monitoring will expose apis to access performance data in


General tip: Don't indent the nested lists.

brainfunked · 2016-12-15T12:55:23Z

specs/time_series_api.adoc

+        touching any other part of the code.
+    *** The way the plugins work is :
+      **** The interfaces to time-series db are added under the directory
+           tendrl/performance-monitoring/time_series_db/dbplugins of


In that case, omit the tendrl/performance-monitoring prefix from the directory name, since you've mentioned the repository name at the end of the sentence anyway.

brainfunked · 2016-12-15T12:56:19Z

specs/time_series_api.adoc

+            ***** Since the manager is a singleton, there is only one instance
+                  of it throughout the life of Tendrl/performance-monitoring
+                  application which means all this house-keeping is done only
+                  once during the lifecycle of the application.


brainfunked · 2016-12-15T12:59:07Z

specs/time_series_api.adoc

+Sample Request
+
+----------
+GET /monitoring/nodes/<node_name>/monitored_types


The node_name will need to be understood by the client making this API call somehow. Aren't the main APIs using node ids? The clients should not have to translate. All our APIs must use the same identifiers for objects.

Basically, the problem is that, the stats are stored in graphite with node names and it is the collectd plugin that decides the name to be associated with a metric its like chosen prefix.node_name.resource_type.stat_type . If only we override the custom collectd plugins and write our own plugins, we'll be able to customize this name. But anyway instead of the api user having to do the mapping, the performance_monitoring application can as well do it using details in etcd. I'll modify the spec to reflect this

brainfunked · 2016-12-15T13:00:34Z

specs/time_series_api.adoc

+----------
+
+The above response gives the statistics for last 24 hours corresponding to the
+'resource_name' passed to it. If a time range is passed as optional arguements


There would be a maximum time range for which the data is stored in the time series db. Even if this isn't configurable right now, this value needs to be made known somehow.

And speaking of which, what's the way to change the duration for which the data would be stored in the db? Can this be done on the fly while the system is still pushing updates to the time series db?

I have not thought of this before. I'll explore if there's a way to do this

Does graphite provide some sort of archiving mechanism and data which is older than say 2 yrs goes to archive??

@shtripat I think I have seen something related to this. I'll explore more on this.
@shtripat @brainfunked can detailing on this go into a separate spec? As this spec is more about the procedure employed to access time series data and hence mainly exposing apis to do the same. I'll create a bug to track that and add a mention of that here.

@anmolbabu I am ok fro different spec

Raised an issue to track this : #99

brainfunked · 2016-12-15T13:02:31Z

specs/time_series_api.adoc

+
+----
+[time_series]
+time_series_db = graphite


Do we really need to expose this option? Do we even support any db other than whisper?

The plan is to be able to support multiple time series dbs although there will be only 1 officially recommended. Any way its pluggable so the moment we add a plugin to different db, there'll be 2 types of dbs of which any one can be used. So, we have this field.

brainfunked · 2016-12-15T13:02:48Z

specs/time_series_api.adoc

+[time_series]
+time_series_db = graphite
+time_series_db_server = 0.0.0.0
+time_series_db_port = 80


Does the API run as root? It must not.

That's not actually a necessity. I just tried running flask the tool that we use for apis as non-root, non-sudo user and it works. And the graphite querying part also can work as any user its just a http get.

brainfunked · 2016-12-15T13:03:40Z

specs/time_series_api.adoc

+
+=== Work Items:
+
+* https://github.com/Tendrl/performance_monitoring/issues/7


@anivargi please create an issue on the api repository and link it here.

@brainfunked Tendrl/api#43
I will write a specification before we start implementing this.

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Add security details + Details about monitoring-core api interactions tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

nthomas-redhat · 2016-12-16T07:30:04Z

specs/time_series_api.adoc

+Making call's to the monitoring app is via a http client programatically. The
+tendlr-api will define its own url's, somewhat similar to what API's from the
+monitoring app. We need to take this approach since we need to have one point
+of interaction.


I guess what you mentioned here was the APIs exposed by monitoring app. Its good to mention them as well here becuse, those are the one's exposed to the end user

@anivargi Could you please help me add it

nthomas-redhat · 2016-12-16T07:43:14Z

specs/time_series_api.adoc

+* The default login auth cookie is alive for 365 days unless user explicitly
+  logged out. But this cookie alive period is exposed for configuration by
+  flask as a field by name 'REMEMBER_COOKIE_DURATION'.
+


Does flask support token based auth?

@nthomas-redhat As we discussed offline, this needs to be inline with the approach taken by tendrl-core api. So, I'll raise a different bug and correspondingly a different spec to address this.

As discussed offline I have added a reference to github issue that will address the auth part of monitoring stack

The issue is @ #89

brainfunked · 2016-12-19T11:49:36Z

specs/time_series_api.adoc

+To get the statistics for a given time-range:
+
+----------
+GET /monitoring/nodes/<node_id>/<resource_name>/stats?from=1997-07-16T19:20+01:00&to=1997-07-18T19:20+01:00


What about time zones?

The time format employed is ISO 8601. I think this is standard followed by tendrl-api as well

So in case of 1997-07-18T19:20+01:00, +01:00 is offset from UTC time so with UTC as base time and offset mentioned the problem of time-zones is overcome

@anivargi could you please add if I am missing something

@anmolbabu yes, but is it possible for queries to be time zone specific and will they be resolved automatically and correctly or do we need to implement some UTC based time zone conversion to resolve such queries?

@brainfunked , The conversion needs to be implemented by us.

Always use aware datetime object, i.e. with timezone information. That makes sure you can compare them directly (aware and unaware datetime objects are not comparable) and will return them correctly to users. Leverage "pytz" to have timezone objects.

Use ISO 8601 as input and output string format. Use datetime.datetime.isoformat() to return timestamps as string formatted using that format, which includes the timezone information.

More here: https://julien.danjou.info/blog/2015/python-and-timezones

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

nthomas-redhat · 2016-12-19T15:29:13Z

Looks good to me

shtripat

LGTM. One question from @brainfunked need to be answered.

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Specification for exposing apis to access time-series data

4e732e8

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

shtripat reviewed Dec 14, 2016

View reviewed changes

Linked github component issue

0487718

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

shtripat reviewed Dec 15, 2016

View reviewed changes

r0h4n suggested changes Dec 15, 2016

View reviewed changes

nthomas-redhat requested changes Dec 15, 2016

View reviewed changes

brainfunked suggested changes Dec 15, 2016

View reviewed changes

anmolbabu added 3 commits December 15, 2016 19:53

Incorporated comments

12fd5b7

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Incorporated comments

054b397

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

Add security details + Details about monitoring-core api interactions

374690a

Add security details + Details about monitoring-core api interactions tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

nthomas-redhat reviewed Dec 16, 2016

View reviewed changes

brainfunked reviewed Dec 19, 2016

View reviewed changes

Add security spec issue reference here

de012d4

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

nthomas-redhat approved these changes Dec 19, 2016

View reviewed changes

shtripat approved these changes Dec 20, 2016

View reviewed changes

Add a note on time zone conversions in api

7784cea

tendrl-bug-id: Tendrl#62 Signed-off-by: anmolbabu <anmolbudugutta@gmail.com>

r0h4n approved these changes Jan 11, 2017

View reviewed changes

r0h4n merged commit ee0ae90 into Tendrl:master Jan 11, 2017

		=== Assignee(s):


		Who is leading the writing of the code? Or is this a spec where you're


		=== Security impact:

		The apis exposed by Tendrl/performance-monitoring although are only getters,


		==== Tendrl API impact:

		The tendrl-api interfaces to the apis mentioned above under the section


		== Proposed change

		* Tendrl/performance-monitoring will expose apis to access performance data in


		=== Work Items:

		* https://github.com/Tendrl/performance_monitoring/issues/7

Specification for exposing apis to access time-series data #63

Specification for exposing apis to access time-series data #63

Conversation

anmolbabu commented Dec 13, 2016

anmolbabu commented Dec 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shtripat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolbabu Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolbabu Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolbabu Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brainfunked left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anmolbabu Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nthomas-redhat commented Dec 19, 2016

shtripat left a comment

Choose a reason for hiding this comment

anmolbabu commented Dec 13, 2016 •

edited

Loading

anmolbabu Dec 15, 2016 •

edited

Loading

anmolbabu Dec 15, 2016 •

edited

Loading

anmolbabu Dec 15, 2016 •

edited

Loading

anmolbabu Dec 16, 2016 •

edited

Loading