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

#2714 #2750

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

saidi-adot
Copy link

@saidi-adot saidi-adot commented Mar 10, 2023

Add support for connection string to Microsoft.ApplicationInsights.NLogTarget #2714

Fix Issue # .

Changes

(Please provide a brief description of the changes here.)

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

Add support for connection string to Microsoft.ApplicationInsights.NLogTarget microsoft#2714
@saidi-adot
Copy link
Author

@saidi-adot please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="ADOT MVM"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (ADOT MVM) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="ADOT MVM"

Contributor License Agreement

@saidi-adot saidi-adot closed this Mar 13, 2023
@saidi-adot saidi-adot reopened this Mar 13, 2023
@saidi-adot

This comment was marked as duplicate.

@saidi-adot
Copy link
Author

saidi-adot commented Mar 13, 2023 via email

@saidi-adot
Copy link
Author

@microsoft-github-policy-service agree
and
@microsoft-github-policy-service agree company="ADOT MVM"

@saidi-adot
Copy link
Author

@snakefoot - Thank you for approving changes. Can you please approve the PR to merge the code.

@snakefoot
Copy link
Contributor

snakefoot commented Sep 23, 2023

@saidi-adot Only repository owners like @TimothyMothra, @cijothomas or @teo-tsirpanis can merge pull-requests here.

@teo-tsirpanis
Copy link
Contributor

@snakefoot I am not a repository owner. 😅

@saidi-adot
Copy link
Author

@TimothyMothra / @cijothomas - Can you please review the changes and merge the PR.

@saidi-adot
Copy link
Author

@TimothyMothra / @cijothomas - Can you please review this PR

@saidi-adot
Copy link
Author

@cijothomas - I fixed the build issues and committed the new changes. Can you please review now..

@cijothomas
Copy link
Contributor

Given the overall direction is to use OpenTelemetry, I am not sure if we want to keep adding new features to this repo anymore. Though I agree that this is a relatively small fix with valid use case. since ConnectionString is going to be mandatory soon..... I'll defer to @rajkumar-rangaraj and @TimothyMothra to give direction here.

Adding Documentation for Application Insights ConnectionString NLog Target
@PawelAdamczuk
Copy link

I am waiting for this to get merged.

As to comment by @cijothomas, in the project I'm working on, we recently switched from legacy logging to ApplicationInsights, so, at least for us, it's not going away anytime soon.

There's this recommendation in the docs: https://learn.microsoft.com/en-us/azure/azure-monitor/app/opentelemetry-enable?tabs=aspnetcore#should-i-use-opentelemetry-or-the-application-insights-sdk
so I suppose our project is not the only one still opting for AI over OT.

var configuration = TelemetryConfiguration.CreateDefault();
configuration.ConnectionString = connectionString;

this.telemetryClient.TelemetryConfiguration.ConnectionString = connectionString;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be tricky... Will this affect the TC.Active? Or this will only affect the telemetryclient owned by nlogtarget? The ikey approach only affected own telemetryclient, and nothing else.

Copy link
Author

Choose a reason for hiding this comment

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

@cijothomas - this.telemetryClient field is local for this Microsoft.ApplicationInsights.NLogTarget.ApplicationInsightsTarget class. Is there any other code to refer to for the iKey approach?

Resolving review comments
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.

5 participants