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

NSQ Output plugin #325

Closed
wants to merge 4 commits into from
Closed

NSQ Output plugin #325

wants to merge 4 commits into from

Conversation

jrxFive
Copy link
Contributor

@jrxFive jrxFive commented Oct 27, 2015

NSQ output plugin #309, following the NSQ methodology output is a producer
to one instance of NSQD. The go library does not accept array values. String
default for a Producer. Additionally service discovery is generally
done as a consumer.

Follows same methodology as Kafka Output without the tag reference.

NSQ output plugin, following the NSQ methodology output is a producer
to one instance of NSQD. The go library does not accept array values be
default for a Producer. Additionally service discovery is generally
done as a consumer.

Follows same methodology as Kafka Output without the tag reference.
@sparrc
Copy link
Contributor

sparrc commented Oct 27, 2015

@jrxFive You've written this based on an outdated version of telegraf. The Write function doesn't take BatchPoints anymore, it takes []*client.Point

You should rebase your changes from HEAD, you'll see that the kafka output is quite a bit different too. It should actually simplify the Write function, because you will not need to deal with a .Raw or any Tag inheritance and parsing.

@sparrc
Copy link
Contributor

sparrc commented Oct 27, 2015

@jrxFive Thank you very much for this! It looks good other than needing to be rebased

@jrxFive
Copy link
Contributor Author

jrxFive commented Oct 27, 2015

Sorry should have done that prior. It's a lot simpler than before :). I'll commit the change soon.

image: nsqio/nsq
ports:
- "4150:4150"
- "4151:4151"
Copy link
Contributor

Choose a reason for hiding this comment

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

What does port 4151 do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

4150 is an TCP listener port, 4151 is for HTTP even though the plugin isnt using that feature should I remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's fine, you can keep it

@sparrc
Copy link
Contributor

sparrc commented Oct 27, 2015

@jrxFive did you run the NSQ docker test? I'm getting a connection refused failure on OSX:

2015/10/27 13:12:38 INF    1 (192.168.99.100:4150) connecting to nsqd
2015/10/27 13:12:38 ERR    1 (192.168.99.100:4150) error connecting to nsqd - dial tcp 192.168.99.100:4150: getsockopt: connection refused
--- FAIL: TestConnectAndWrite (0.00s)
        Error Trace:    nsq_test.go:27
    Error:      No error is expected but got FAILED to send NSQD message: dial tcp 192.168.99.100:4150: getsockopt: connection refused

FAIL
FAIL    github.com/influxdb/telegraf/outputs/nsq    0.018s

@jrxFive
Copy link
Contributor Author

jrxFive commented Oct 27, 2015

Hmm I ran tests on a Linux VM and directly against a NSQ daemon. Weird I'll try on OSX to reproduce.

@sparrc
Copy link
Contributor

sparrc commented Oct 27, 2015

@jrxFive I'm trying to debug, for some reason the NSQ container is not starting for me

@sparrc
Copy link
Contributor

sparrc commented Oct 27, 2015

It looks like we're missing the "command", it needs to look like this in docker-compose

nsq:
    image: nsqio/nsq
    command: /nsqd
    ports:
        - "4150:4150"
        - "4151:4151"

@sparrc
Copy link
Contributor

sparrc commented Oct 27, 2015

@jrxFive I'll make the change and merge this

@jrxFive
Copy link
Contributor Author

jrxFive commented Oct 27, 2015

Ahh good catch! Hmm why didnt this fail on the Circle-CI test? Its using Ubuntu Circle-CI? Wonder why OSX caught that though?

@sparrc
Copy link
Contributor

sparrc commented Oct 27, 2015

CircleCI isn't currently running the "long" docker tests, see #293

I need to find some time to get that env working on Circle

@sparrc sparrc closed this in e622bd5 Oct 27, 2015
@sparrc
Copy link
Contributor

sparrc commented Oct 27, 2015

Boy, that became a doozy. I decided to eliminate docker-compose from the unit tests, instead just running the docker containers directly from the Makefile. docker-compose seems to do a lot of caching that I wasn't able to disable.

I never really liked the docker-compose requirement anyways :-)

@sparrc sparrc mentioned this pull request Nov 3, 2015
allenj pushed a commit to allenj/telegraf that referenced this pull request Nov 18, 2015
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.

2 participants