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

New Relic appender #176

Closed
wants to merge 1 commit into from
Closed

New Relic appender #176

wants to merge 1 commit into from

Conversation

polymeris
Copy link
Contributor

No description provided.

@polymeris
Copy link
Contributor Author

Noticed :?err is not assigned unless the exception is passed as first parameter, e.g. (timbre/error "Error doing thing" exception) doesn't work. Is that correct/intended?

@ptaoussanis
Copy link
Member

Is that correct/intended?

@polymeris Hi Camilo, yes - that's correct.

params (HashMap. {"Log level" (name level)})]
(if ?err
(NewRelic/noticeError ?err params)
(NewRelic/noticeError (output-fn data) params))))})
Copy link
Member

Choose a reason for hiding this comment

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

Hi there. Am not at all familiar with NewRelic. Just want to check that it's intentional to call "noticeError" even when there's no error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's intentional. noticeError can take a string or an exception as first parameter.
Semantically, is arguable wether it makes sense, but to me there is an error if you call a timbre logging function, and it should be reported, even if there is no associated exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the clarification.

So this NewRelic API doesn't have a notion of logging anything besides an error? The idea here is that this appender intentionally only logs errors?

(NewRelic/noticeError ?err params)

Is there a reason you don't want more of the data content for this call?

All you're be logging in this case is the exception and level. Not the namespace, logging message, args, etc.

Any reason you wouldn't want, for example, to add (output-fn data) to the params map here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this NewRelic API doesn't have a notion of logging anything besides an error?

No other messages, as far as I can tell. It logs transactions (web, db, and others), but that's a different story.

All you're be logging in this case is the exception and level. Not the namespace, logging message, args, etc.

The stack trace (and thus the namespace) is logged.

Sure we could log more stuff. TBH, I haven't even figured out how to get New Relic to show those "params" you pass to noticeError. It mostly cares about: exception class, exception message, and the containing transaction (a function which you can designate as such, usually).

There is also the notion of "attributes". I might explore those in the future as a way to add more info to error logging, but for the time being I only wanted the basics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. It's true that NR is not a logging facility.

That said, for me, it's just practical that at the very least error and fatal should go trough New Relic (and that's what I actually do in my project, not sure why I specified :warn as the default). Sure I could call noticeError directly, but if I already have the timbre hook, and the cases where I log an error (i.e. when the transaction or job failed) are exactly the same I would report to NR... why not use that?

Copy link
Member

Choose a reason for hiding this comment

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

the cases where I log an error (i.e. when the transaction or job failed) are exactly the same I would report to NR... why not use that?

Because you'd be complecting things that are semantically very different:

  1. Reporting an API-specific error to a particular API.
  2. General-purpose, multi-level logging to an arbitrary set of appenders.

So there's two questions here. First: is this the best approach to take in your specific case? I'd argue not, when you can just do this with a function that sends the notification and triggers a related Timbre logging call if you like. That way you're not tying certain logging levels into a specific semantic meaning. What if you call Timbre's error function somewhere that has nothing to do with this NewRelic library?

Second question: is this going to be widely useful to enough other developers to warrant inclusion in Timbre itself, ongoing maintenance, etc.? Do you see many other users wanting this particular behaviour?

Does that make sense / seem reasonable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the first question, I understand your point, but I don't see how I could not want all errors to be recorded by New Relic. Or conversly, if there were things I did not want sent to New Relic, I'd argue they are not errors, and I would be abusing timbre/error by logging them with it. (I'll need to think about this point, tho)
Slack isn't a logging app either, but I still want my errors to be sent there, too.

Regarding the second question... I don't know. If you are not convinced, maybe we can just close this PR. We can always reopen it later if the need for this appender becomes evident. Thank you for your patience.

Copy link
Member

Choose a reason for hiding this comment

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

but I don't see how I could not want all errors to be recorded by New Relic.

Okay, so it actually makes sense to send all application errors from any source to New Relic? It's a little difficult from https://newrelic.com/ to figure out what this software actually does. Looked to me like some kind of metrics library but I must be misunderstanding?

What happens with the info that you report with NewRelic/noticeError?

To be absolutely clear:

  1. It makes sense to send every application-level error to NewRelic even if the error has nothing to do with NewRelic itself?
  2. It makes sense to send only error-level information; there's no analog for other logging levels?

Thank you for your patience.

No problem, and likewise thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense to send every application-level error to NewRelic even if the error has nothing to do with NewRelic itself?

Yes, New Relic is a monitoring and analytics tool. They provide error analytics, among other things. So one can look at pretty graphs about which hosts suffer how many errors of this or that class at this or that time of the day, during this or that transaction/request/job.

It makes sense to send only error-level information; there's no analog for other logging levels?

As far as I can tell, there are no other log levels (I am no expert, we only started using this thing).

@ptaoussanis ptaoussanis force-pushed the master branch 3 times, most recently from 537eb30 to 2329d59 Compare July 4, 2016 10:20
@ptaoussanis
Copy link
Member

Okay, thanks for clearing the confusion. Will merge!

@ptaoussanis ptaoussanis closed this Jul 5, 2016
ptaoussanis pushed a commit that referenced this pull request Jul 5, 2016
ptaoussanis added a commit that referenced this pull request Jul 5, 2016
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