winstonjs / node-loggly

A client implementation for Loggly in node.js
http://github.com/winstonjs/node-loggly
Other
233 stars 80 forks source link

tags are not sent when doing bulk / array #35

Closed christiaanwesterbeek closed 10 years ago

christiaanwesterbeek commented 10 years ago

Logging an array makes use of the Bulk Endpoint of Loggly's HTTP API. However tags are not being send over. (Using node-loggly 1.0.3)

var loggly = require('loggly');

var client = loggly.createClient({
  token     : '12345678-1234-1234-1234-123456789ab',
  subdomain : 'abc'
});

client.log('single', ['test']);
client.log(['mul', 'ti'], ['test']);

image note that the tags are missing with mul and ti.

I tried the same thing with Fiddler and that works, so Loggly Bulk Endpoint is not broken.

image

image

freeall commented 10 years ago

In our setup we send our environment as a tag to loggly, so for us it works.

Does it work if you don't parse the tag as an array? Like client.log(['mul', 'ti'], 'test');

christiaanwesterbeek commented 10 years ago

Thanks for trying. I tried that too just now.

client.log('single', ['test']);
client.log(['mul', 'ti'], ['test']);
client.log(['mul2', 'ti2'], 'test');

image

I have debugged al the way through Request. The header x-loggly-tag seems to be correctly prepared by node-loggly before the request is actually made. So it puzzles may to see the exact same thing in Fiddler working. Btw I'm on Windows 8.

freeall commented 10 years ago

Sorry, I'm most likely not able to fix it these days. Are you able to fix it yourself? Otherwise I hope someone else can do it.

christiaanwesterbeek commented 10 years ago

I will try but expect to have to go deep, because I suspect a problem in one of the modules node-loggly depends on.

jcrugzz commented 10 years ago

This is super weird. I have been slamming loggly with messages (both bulk and individual packets) but I have not seen this type of behavior (as they ALL have tags). I will see if there is anywhere in my history where things have been going in tagless but this doesn't seem likely. @devotis what node version are you using? It is possible this has something to do with node on windows.

christiaanwesterbeek commented 10 years ago

Node v 0.10.3 on Windows 8.1. I inspected all the way through Request to find out that the headers were actually prepared right while doing bulk log (array). No tags in Loggly. Redid it with Fiddler. Boom, tags!

I find it hard to believe that there would be a bug in Node or a very common module like request. But there's no bug in node-loggly either, because the headers in bulk versus simple are completely the same. I haven't had the time to dive deep yet.

(Update: I was/am on node v0.10.23 not v0.10.3)

freeall commented 10 years ago

@devotis if you could take some screenshots of what you find when go down to the request-layer, or do it in Fiddler we could try to take a look at it. But yeah, it seems unlikely to be a bug in node code or in request - never say never, though :)

christiaanwesterbeek commented 10 years ago

Will do just that. Just give me same days to find time.

jcrugzz commented 10 years ago

@devotis you should definitely be using 0.10.24. Try upgrading your node and see if that helps!

christiaanwesterbeek commented 10 years ago

Here's my findings with screenshots. I'm on node v0.10.23 on Windows 8.1 32bit. Will upgrade node after I'm done reproducing the error with my current node version.

My App: image

Depends on loggly 1.03 that depends on request 2.27.0 image

I have inspected just before a new Request is made in node_modules\request\index.js image

Now I run... node app2 image

And I see correct headers with every request. What's next to try?... I can upgrade node now, but I wish to see what's causing this.

BTW: proof that just now no tags are logged when doing bulk: image

I tried the same thing with Fiddler and that works, so Loggly Bulk Endpoint is not broken.

viggeh commented 10 years ago

I've noticed the same problem but I did observe that by adding the tags to the URL instead of the HTTP header will cause loggly to respect the tags. So by changing the following lines in lib/loggly/client.js (line 130)

if (tags) {
    logOptions.headers['x-loggly-tag'] = tags.join(',');
  }

to

if (tags) {
    //logOptions.headers['x-loggly-tag'] = tags.join(',');
    logOptions.uri += "/tag/" + tags.join(",") + "/";
  }

The tags will show up in loggly for bulk inserts.

viggeh commented 10 years ago

I added a pull request with a workaround for this problem.

jcrugzz commented 10 years ago

@viggeh thanks for the workaround! Seems like both methods are valid ways to send tags but that is totally a weird bug on loggly's part if headers are not handled consistently on bulk requests. Workaround published as 1.0.4.