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

feat: NLog support #993

Merged
merged 5 commits into from
May 24, 2019
Merged

feat: NLog support #993

merged 5 commits into from
May 24, 2019

Conversation

bruno-garcia
Copy link
Member

@bruno-garcia bruno-garcia commented May 20, 2019

Docs to NLog integration brought in by @josh-degraw
@josh-degraw Could you please also review it?

@bruno-garcia
Copy link
Member Author

Package still beta so build isn't happy about it since it can't find it in the registry.
I'll publish it next week so once the PR is approved I can merge it right after releasing it.

Copy link
Contributor

@josh-degraw josh-degraw left a 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" />
Copy link
Contributor

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.

Copy link
Member Author

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___"
Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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.

src/collections/_documentation/platforms/dotnet/nlog.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MimiDumpling MimiDumpling left a 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!

src/collections/_documentation/platforms/dotnet/nlog.md Outdated Show resolved Hide resolved
src/collections/_documentation/platforms/dotnet/nlog.md Outdated Show resolved Hide resolved
src/collections/_documentation/platforms/dotnet/nlog.md Outdated Show resolved Hide resolved
bruno-garcia and others added 3 commits May 23, 2019 16:20
Co-Authored-By: Tien "Mimi" Nguyen <tienmiminguyen@gmail.com>
Co-Authored-By: Tien "Mimi" Nguyen <tienmiminguyen@gmail.com>
@bruno-garcia bruno-garcia merged commit d73752e into master May 24, 2019
@bruno-garcia bruno-garcia deleted the feat/dotnet-nlog branch May 24, 2019 12:05
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants