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

NLog ApplicationInsightsTarget with support for IncludeMldc #2103

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Oct 30, 2020

Add support for NLog Mapped Diagnostic Logical Context (MDLC)

Changes

  • Updated to NLog 4.7.15 with support for writing into existing property-bag.
  • Inheriting from TargetWithContext

Checklist

  • I ran Unit Tests locally.
  • CHANGELOG.md updated with one line description of the fix, and a link to the original issue if available.

For significant contributions please make sure you have completed the following items:

  • Design discussion issue #
  • Changes in public surface reviewed

The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.

Notes for authors:

  • FxCop and other analyzers will fail the build. To see these errors yourself, compile localy using the Release configuration.

Notes for reviewers:

  • We support comment build triggers
    • /AzurePipelines run will queue all builds
    • /AzurePipelines run <pipeline-name> will queue a specific build

@snakefoot snakefoot changed the title NLog ApplicationInsightsTarget with support for IncludeMldc - WIP NLog ApplicationInsightsTarget with support for IncludeMldc Oct 30, 2020
@304NotModified
Copy link

@TimothyMothra why has this never get merged? Or has received any feedback?

@TheXenocide
Copy link

Should I interpret the staleness of this PR to mean that there is a very low likelihood of #2785 being resolved?

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 11, 2023

I'm guessing Microsoft wanted to invite existing NLog users to try the fruits of ApplicationInsights, which give birth to this NLog Target.

Microsoft is probably putting most of their energy into their pure Microsoft Extension Logging integration with ApplicationInsights, and not any hybrid modes.

Microsoft probably believes that one would not use NLog, after having started using Microsoft Extensions Logging.

@304NotModified
Copy link

Maybe we should fork this repo and release it as a NLog package. Not perfect, but I dont like getting ghosted.

@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 12, 2023

Maybe we should fork this repo and release it as a NLog package. Not perfect, but I dont like getting ghosted.

Would prefer if someone with extensive knowledge about ApplicationInsights (along with its telemetry capture) took up the challenge. Since right now this NLog-target package cannot stand on its own (doesn't capture/forward all application-performance keyfigures).

@TheXenocide
Copy link

From what I could see, I suspect maybe the App Insights team has been focused on other platforms (e.g. Python, etc.) for a little while, plus MS has been dedicating resources to integrating OpenTelemetry (which is a less proprietary design for similar functionality) to both .NET and App Insights so I'm hopeful it has just been a matter of lacking resources to dedicate to this side for now, though it does seem this has been here quite a while. I've also recently learned that .NET 8 is introducing a new Microsoft.Extensions.Telemetry set of abstractions so maybe there are some major refactorings coming? Not sure what to expect, just yet, as there's no documentation or road map about this very recent announcement and I haven't read the source yet.

Either way, without knowing what implementation we could possibly pursue with those things yet, we find ourselves missing needed functionality to properly implement App Insights at the immediate moment, since all of our logging goes through NLog and we don't have a current enough bridge to bring our traces and exceptions from there into App Insights.

@snakefoot snakefoot force-pushed the develop branch 2 times, most recently from 810d9eb to 8948cf1 Compare June 17, 2023 18:47
@snakefoot snakefoot changed the base branch from develop to main June 17, 2023 18:49
@snakefoot
Copy link
Contributor Author

snakefoot commented Jun 17, 2023

See also: direction is to use OpenTelemetry

@TheXenocide
Copy link

The overview linked there has the following excerpt which makes me wonder where we should be focusing our effort for the next release:

While we see OpenTelemetry as our future direction, we have no plans to stop collecting data from older SDKs. We still have a way to go before our Azure OpenTelemetry Distros reach feature parity with our Application Insights SDKs. In many cases, customers will continue to choose to use Application Insights SDKs for quite some time.

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