-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: NLog support #993
feat: NLog support #993
Conversation
9ee58d3
to
8ad4f89
Compare
Package still beta so build isn't happy about it since it can't find it in the registry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just a few comments / requests that I thought of.
</targets> | ||
|
||
<rules> | ||
<logger name="*" writeTo="sentry" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that might be good to super clarify is that as of now, it doesn't use the standard NLog way of specifying the log level.
For example, you typically you set the minlevel
on the rule for the lowest rule you want to be logged, but here there are the MinimumEventLevel
and MinimumBreadcrumbLevel
in addition to the standard. I think it's pretty straight forward and it is explained in these docs, but it might be good to draw attention to this difference here, e.g.:
<!--
NOTE: If the `minlevel` for the logger is higher than `MinimumBreadcrumbLevel`, those messages
won't be added as breadcrumbs.
Set the minlevel as low as possible to allow all messages to
pass through sentry target, and configure levels for breadcrumbs and events above.
-->
<logger name="*" writeTo="sentry" />
<!-- The above is equivalent to <logger name="*" minlevel="Trace" writeTo="sentry"/> -->
It could be worded however, but I think it would be good to make sure that is super duper clear so people can make sure their breadcrumbs are sent to sentry correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a note about this. I tried to be detailed on how it works but still seems confusing. Would appreciate some suggestions.
Or feel free to make a PR on this branch too if you wish :)
|
||
<targets> | ||
<target xsi:type="Sentry" name="sentry" | ||
dsn="___PUBLIC_DSN___" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I just thought of is that I specified that the Dsn
is specified as a required parameter in the SentryTarget
, which means that if a sentry target is defined in the NLog.config
file without a dsn specified, it will probably not register the target correctly.
I'd have to test it to verify that, but that could cause issues if people want to set the dsn another way. So that should either be documented here or else changed on the target, which I'm happy to do if you think that's the better option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. We should make this not required then. Firstly because the lack of DSN
means don't initialize Sentry at all but also because It's important to make it possible for integration not to initialize the SDK. The canonical way to init any Sentry SDK from the unified API is via Sentry.Init
so allowing an integration initialize the SDK is just a convenience for those who use a single integration. It also helps making things idiomatic.
Could you please modify the integration to make DSN
optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do. I should be able to send a PR this weekend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple grammar things. :) Thanks for all your work!
Co-Authored-By: Tien "Mimi" Nguyen <[email protected]>
Co-Authored-By: Tien "Mimi" Nguyen <[email protected]>
Docs to NLog integration brought in by @josh-degraw
@josh-degraw Could you please also review it?