Skip to content

Conversation

@mattcasey
Copy link

In the original code, "meta.tags" never gets used because this.tags is always set.

@jcrugzz
Copy link
Contributor

jcrugzz commented Mar 5, 2015

@mattcasey curious as to what makes this.tags always defined. is it options.id? wondering if ensuring it defaults to an array and concatting the tags. As you may want to set global ones AND tags per message. Thoughts?

@indexzero
Copy link
Member

@indexzero
Copy link
Member

lgtm.

indexzero added a commit that referenced this pull request Mar 6, 2015
Allow setting tags per log statement
@indexzero indexzero merged commit 9741c36 into winstonjs:master Mar 6, 2015
@jcrugzz
Copy link
Contributor

jcrugzz commented Mar 6, 2015

@indexzero i still think we may want to concat in this case rather than override.

@indexzero
Copy link
Member

mmm ... idk. Semantically I could see how that would bother me.

@indexzero
Copy link
Member

e.g. "I only want to send THESE tags, nothing else!"

@jcrugzz
Copy link
Contributor

jcrugzz commented Mar 6, 2015

@indexzero eh i guess my thought there is that IF you were to define global tags, i may want to add custom tags inheriting those global ones for different type of log messages. But since it defaults to the ID i could see it being less of an expectation.

@jcrugzz
Copy link
Contributor

jcrugzz commented Mar 6, 2015

but im biased as this is how i made the node-loggly client work internally since you only explicitly provide global tags as well as per Log message tags

@indexzero
Copy link
Member

yeah, it's also likely you keep them in the outer scope when you pass them into the Loggly transport instance. e.g.:

var winston = require('winston'),
    Loggly = require('winston-loggly');

var tags = ['this', 'that', 'the-other'];

var logger = new winston.Logger({
  transports: [
    new Loggly({ tags: tags })
  ]
});

logger.info('hey, a log!', { 
  tags: tags.concat(['custom-tags', 'over-here', 'with-default-tags-too'])
});

@jcrugzz
Copy link
Contributor

jcrugzz commented Mar 6, 2015

@indexzero well i do believe we may have differing assumptions as this still feels weird to me. Since the pull-request was for this way, we can leave it at that ;).

@indexzero
Copy link
Member

@jcrugzz I didn't know that about the node-loggly client. I must've missed it in the v2.0 refactor. Is this what you have in mind? #24

jcrugzz added a commit that referenced this pull request Mar 6, 2015
Use default tags in `node-loggly` client
@mattcasey
Copy link
Author

Hey guys! Thanks for accepting our PR. It didn't cross my mind to allow inheriting the tags. I could live with either way, just so long as we can add specific tags along-side each message :)

OtterCode pushed a commit to OtterCode/winston-loggly that referenced this pull request Jun 22, 2018
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.

3 participants