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

ITelemetryClient to facilitate testing/mocking #2761

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

Conversation

thomhurst
Copy link

@thomhurst thomhurst commented Apr 4, 2023

Fix Issue #342 .

Changes

An interface of ITelemetryClient, containing all the public methods/properties available on TelemetryClient.
This allows consumers to inject an interface, rather than a concrete implementation, into their classes, which makes mocking and testing easier for consumers.

This doesn't change any behaviour around TelemetryClient currently. It just adds the ability to also inject in ITelemetryClient via DI. You can still inject in TelemetryClient. The choice is up to the consumer. Both will work.

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.

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

@304NotModified
Copy link

Great work!

@thomhurst
Copy link
Author

/AzurePipelines run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2761 in repo microsoft/ApplicationInsights-dotnet

Copy link
Contributor

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

We should be very careful adding new public APIs in this repo. The long term goal would be to leverage OpenTelemetry, so try to keep the changes here minimal, unless critical.

@thomhurst
Copy link
Author

We should be very careful adding new public APIs in this repo. The long term goal would be to leverage OpenTelemetry, so try to keep the changes here minimal, unless critical.

It's not really adding an API. It's the exact same API as current, but with the option of calling the existing methods on an interface rather than the concrete implementation.

The long term goal is fine for when it happens, but it's still a WIP and many people are left with using TelemetryClient for now until it's built out a bit more.

This has been an open issue for years and people want to be able to write tests so that they feel comfortable with their logging code.

Can I ask why it's so contentious to put an interface behind the TelemetryClient?

@304NotModified
Copy link

Please accept this small change in code (no breaking changes, no extra public api) that will result in great possibilities with automatic testing.

@304NotModified
Copy link

Polite bump @cijothomas. There is no change needed? The public api hasn't changed

@304NotModified
Copy link

@TimothyMothra could please help us in getting this PR merged? (you're the top contributor of this repo)

Copy link

@aligneddev aligneddev left a comment

Choose a reason for hiding this comment

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

This looks good to me. I was hopping their was already an interface. This change would have simplified my mocking

@thomhurst
Copy link
Author

Unfortunately Microsoft don't seem to be willing to merge this for some reason, so I don't think you'll have any luck

@aligneddev
Copy link

@thomhurst at least the example in #342 was straight forward and I got it to work. Thanks for trying.
Happy Thanksgiving! 🦃

@valters-tomsons
Copy link

It would be very useful to have this PR merged, it makes little sense having to touch telemetry channels in some use-cases.

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