Enable streaming collector #85

jcooklin · 2017-05-11T03:23:04Z

The docker-compose file and task used in this demo can be found here.

TODO:

update example (currently hits are not reflected)

IzabellaRaulin · 2017-05-17T17:16:37Z

v1/plugin/stream_collector_proxy.go

+		if err != nil {
+			log.Fatal(err)
+		}
+	}()


@jcooklin, could you provide me more details regardings those changes in func (p *StreamProxy) StreamMetrics()? It seems that this cause that streaming rand collector does not work as expected - running task still has Hits = 0.

IzabellaRaulin · 2017-06-06T10:01:54Z

v1/plugin/plugin.go

-	app = cli.NewApp()
-	app.Flags = []cli.Flag{
+	// Flags required by the plugin lib flags - plugin authors can provide their
+	// own flags.


It would be great to point here to snap-plugin-collector-rand/rand/rand.go as a place where the example of usage (I mean adding a flag in the plugin) can be found.

IzabellaRaulin · 2017-06-06T11:03:28Z

v1/plugin/stream_collector_proxy.go

+
+			s, err := stream.Recv()
+			if err != nil {
+				log.Debug(err)


@jcooklin , could you confirm this log should stay on debug level, or maybe "error" would be more appropriate here? What do you think about keeping the same pattern and defining log with the field "_block".

IzabellaRaulin · 2017-06-06T13:50:18Z

v1/plugin/plugin.go

@@ -606,6 +617,26 @@ func printPreambleAndServe(srv server, m *meta, p *pluginProxy, port string, isP
 	return string(preambleJSON), nil
 }

+// getAddr if we were provided the addr 0.0.0.0 we need to determine the
+// address we will advertise to the framework in the premable.


typo in "premable" -> "preamble"

IzabellaRaulin · 2017-06-06T14:05:33Z

I noticed that I got log error twice:

$ ./snap-streaming --addr 119.50.30.3 --port 30 --stand-alone --stand-alone-port 8185 --log-level 5

ERRO[0000] listen tcp 119.50.30.3:30: bind: cannot assign requested address
ERRO[0000] listen tcp 119.50.30.3:30: bind: cannot assign requested address

I found out that this line duplicates log from here. @jcooklin, I will prepare a PR to your repo with the fix of this small issue
-> jcooklin#7

IzabellaRaulin · 2017-06-06T14:23:31Z

v1/plugin/plugin.go

@@ -573,7 +580,7 @@ func printPreambleAndServe(srv server, m *meta, p *pluginProxy, port string, isP
 	}
 	l.Close()

-	addr := fmt.Sprintf("127.0.0.1:%v", l.Addr().(*net.TCPAddr).Port)
+	addr := fmt.Sprintf("%s:%v", grpcListenAddr, l.Addr().(*net.TCPAddr).Port)


I am pondering if lines above (577-581) should be also refactored based on the fact that determined own grpcListenAddress (different than localhost) is now possible.

This is the scenarios that I've tested.:
a) (OK) set unavailable address with port 30 (notice that the port is available on my localhost):

$ ./snap-streaming --addr 119.50.30.3 --port 30 (of course in standalone mode) ERRO[0000] listen tcp 119.50.30.3:30: bind: cannot assign requested address _block=StartStreanCollector

b) (NOT AS EXPECTED) set unavailable address with port 8181 (notice that the port is already in use on my localhost):

$ ./snap-streaming --addr 119.50.30.3 --port 8181 ERRO[0000] listen tcp :8181: bind: address already in use

In this "b" case I expected to get listen tcp 119.50.30.3:8181: bind: cannot assign requested address. In this case checking if provided port is available or not in localhost should not happen.

IzabellaRaulin · 2017-06-06T17:04:32Z

@jcooklin - is there any plan to refactor examplary stest-rand-streamer? Together with these changes, the plugin does not work as expected - no Hits, no Misses

jcooklin · 2017-06-06T18:10:00Z

is there any plan to refactor examplary stest-rand-streamer?

@IzabellaRaulin: Yes, before it lands the existing example should work as expected.

IzabellaRaulin · 2017-06-06T18:22:05Z

Yes, before it lands the existing example should work as expected

It's great. Could you add "todo" checklist with that in the pull request description - if there are any other things which need to be done before it lands, please add them as well. Thanks

marcintao · 2017-06-08T13:47:48Z

v1/plugin/plugin.go

 	resp := preamble{
 		Meta:          *m,
-		ListenAddress: addr,
+		ListenAddress: fmt.Sprintf("%v:%v", advertisedAddr, l.Addr().(*net.TCPAddr).Port),


Shouldn't the listening address be separate from advertised address?

Here grpcListenAddr (--addr) is used both as -advertised address (reported in preamble) and -listening address net.Listen("tcp", addr).
It does matter in case a qualified domain name is added (in contrast to giving an ip). Then user doesn't have explicit control over address resolution, see also description for net.Listen.

marcintao · 2017-06-09T11:40:28Z

@jcooklin it came to me that default port for plugin advertisement is the same as snapteld's default API port ==8181. Just to be sure: was it intentional to start http listeners on the same port for framework and plugins?

jcooklin · 2017-06-12T17:27:07Z

@marcintao this is addressed in 1e1089e.

jcooklin · 2017-06-16T03:47:03Z

258efba updates the example.

IzabellaRaulin · 2017-06-16T12:48:44Z

examples/snap-plugin-collector-rand-streaming/main.go

@@ -20,7 +20,7 @@ limitations under the License.
 package main

 import (
-	"github.com/intelsdi-x/snap-plugin-lib-go/examples/streaming/rand"
+	"github.com/intelsdi-x/snap-plugin-lib-go/examples/snap-plugin-collector-rand-streaming/rand"


Based on that, some changes are needed in links in README.md - for example here: https://github.com/intelsdi-x/snap-plugin-lib-go/blob/master/examples/streaming/README.md#ready-to-share:

@jcooklin, I made PR with fix to your repo: jcooklin#10

IzabellaRaulin · 2017-06-16T14:10:36Z

I do not why the behavior of aggregation by maxMetricsBuffer has changed - it does not work as I expected (and it does not work as before). As a user, I expect to get aggregated metrics when I request to collect single metrics and set maxMetricsBuffer to 2 (assuming that maxCollectDuration is not exceeded), I expect to get 2 metrics with different timestamp sending to snap framework together, not one by one. Below is a task manifest which I used:

{
    "version": 1,
    "schedule": {
        "type": "streaming"
    },
 "start": true,
  "max-failures": 10,
  "max-collect-duration": "10s",
  "max-metrics-buffer": 2,
    "workflow": {
        "collect": {
            "metrics": {
                "/random/integer": {}
            },
            "publish": [
        {
          "plugin_name": "file",
          "config": {
            "file": "/tmp/published_streaming.log"
          }
        }
           ]
        }
    }
}

IzabellaRaulin · 2017-06-16T14:28:20Z

Ok, so then it LGTM.

IzabellaRaulin

LGTM, please squash your commit into single one

jcooklin requested a review from IzabellaRaulin May 11, 2017 03:23

jcooklin force-pushed the fb/enables-streaming branch from 5683141 to 234758e Compare May 11, 2017 03:26

jcooklin mentioned this pull request May 11, 2017

Adds support for streaming collectors intelsdi-x/snap#1632

Merged

IzabellaRaulin reviewed May 17, 2017

View reviewed changes

jcooklin force-pushed the fb/enables-streaming branch from 234758e to 4f3438e Compare May 19, 2017 05:45

katarzyna-z mentioned this pull request May 29, 2017

Snapteld crashes when a task for a stream collector is created intelsdi-x/snap#1650

Closed

jcooklin force-pushed the fb/enables-streaming branch 2 times, most recently from 06336aa to 57a59d6 Compare June 1, 2017 23:48

IzabellaRaulin reviewed Jun 6, 2017

View reviewed changes

marcintao reviewed Jun 8, 2017

View reviewed changes

kjlyon mentioned this pull request Jun 12, 2017

Make Relay a Collector Plugin intelsdi-x/snap-relay#3

Merged

jcooklin force-pushed the fb/enables-streaming branch from c03dbcf to d9fad5f Compare June 13, 2017 14:53

IzabellaRaulin reviewed Jun 16, 2017

View reviewed changes

IzabellaRaulin approved these changes Jun 16, 2017

View reviewed changes

jcooklin force-pushed the fb/enables-streaming branch from 7e1f8c8 to 0e78d6b Compare June 16, 2017 23:54

Adds support for streaming collectors

3a91475

jcooklin force-pushed the fb/enables-streaming branch from 0e78d6b to 3a91475 Compare June 17, 2017 00:11

jcooklin merged commit 25d0f6c into intelsdi-x:master Jun 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable streaming collector #85

Enable streaming collector #85

jcooklin commented May 11, 2017 •

edited

Loading

IzabellaRaulin May 17, 2017

IzabellaRaulin Jun 6, 2017 •

edited

Loading

IzabellaRaulin Jun 6, 2017

IzabellaRaulin Jun 6, 2017

IzabellaRaulin commented Jun 6, 2017 •

edited

Loading

IzabellaRaulin Jun 6, 2017 •

edited

Loading

IzabellaRaulin commented Jun 6, 2017

jcooklin commented Jun 6, 2017

IzabellaRaulin commented Jun 6, 2017 •

edited

Loading

marcintao Jun 8, 2017

marcintao commented Jun 9, 2017

jcooklin commented Jun 12, 2017

jcooklin commented Jun 16, 2017

IzabellaRaulin Jun 16, 2017

IzabellaRaulin Jun 16, 2017

IzabellaRaulin commented Jun 16, 2017

IzabellaRaulin commented Jun 16, 2017

IzabellaRaulin left a comment

Enable streaming collector #85

Enable streaming collector #85

Conversation

jcooklin commented May 11, 2017 • edited Loading

IzabellaRaulin May 17, 2017

Choose a reason for hiding this comment

IzabellaRaulin Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

IzabellaRaulin Jun 6, 2017

Choose a reason for hiding this comment

IzabellaRaulin Jun 6, 2017

Choose a reason for hiding this comment

IzabellaRaulin commented Jun 6, 2017 • edited Loading

IzabellaRaulin Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

IzabellaRaulin commented Jun 6, 2017

jcooklin commented Jun 6, 2017

IzabellaRaulin commented Jun 6, 2017 • edited Loading

marcintao Jun 8, 2017

Choose a reason for hiding this comment

marcintao commented Jun 9, 2017

jcooklin commented Jun 12, 2017

jcooklin commented Jun 16, 2017

IzabellaRaulin Jun 16, 2017

Choose a reason for hiding this comment

IzabellaRaulin Jun 16, 2017

Choose a reason for hiding this comment

IzabellaRaulin commented Jun 16, 2017

IzabellaRaulin commented Jun 16, 2017

IzabellaRaulin left a comment

Choose a reason for hiding this comment

jcooklin commented May 11, 2017 •

edited

Loading

IzabellaRaulin Jun 6, 2017 •

edited

Loading

IzabellaRaulin commented Jun 6, 2017 •

edited

Loading

IzabellaRaulin Jun 6, 2017 •

edited

Loading

IzabellaRaulin commented Jun 6, 2017 •

edited

Loading