Skip to content

feat: create new telemetry handle that supports connection strings #3729

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BeegiiK
Copy link

@BeegiiK BeegiiK commented Jun 11, 2025

Reason for Change:

Currently, the telemetry handle can only work with instrumentation keys. If we want to use application insights in sovereign clouds, this would simply fail as the logging is shipped to a default ingestion endpoint and the isPublicEnvironment check.

This PR introduces a new telemetry handle method that takes in a connection string, parses it, updates the configuration appropriately and initializes the new handle

Tested manually by updating DNC to use the new telemetry handle and verified that the logs were shipped to Kusto, with the connection string of the test application insights resource

Due to the original ApplicationInsights-Go repo not being maintained, essential changes to support connection strings could not be merged. Therefore, the source code along with the additional changes were brought over as a separate module

Issue Fixed:

#3730

Requirements:

Notes:

@Copilot Copilot AI review requested due to automatic review settings June 11, 2025 08:35
@BeegiiK BeegiiK requested a review from a team as a code owner June 11, 2025 08:35
@BeegiiK BeegiiK requested a review from kmurudi June 11, 2025 08:35
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new telemetry client that supports connection strings for initializing Application Insights telemetry, along with corresponding tests.

  • Updated go.mod to replace the ApplicationInsights-Go dependency.
  • Added a new function, NewAITelemetryWithConnectionString, in telemetrywrapper.go to enable connection string based initialization.
  • Expanded tests in telemetrywrapper_test.go to cover the new connection string initialization.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
go.mod Updated ApplicationInsights-Go dependency replacement.
aitelemetry/telemetrywrapper_test.go Added tests for connection string initialization and error checks.
aitelemetry/telemetrywrapper.go Added NewAITelemetryWithConnectionString function implementation.
Comments suppressed due to low confidence (1)

aitelemetry/telemetrywrapper_test.go:93

  • There is a spelling mistake in the error message; 'intializing' should be changed to 'initializing'.
t.Errorf("Error intializing AI telemetry:%v", err)

@BeegiiK BeegiiK changed the title feat: create new telemetry client with connection strings feat: create new telemetry handle that supports connection strings Jun 11, 2025
@BeegiiK BeegiiK force-pushed the connection-string branch 3 times, most recently from 987145b to fa88a0b Compare June 13, 2025 11:14
Copy link

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Jun 28, 2025
@BeegiiK BeegiiK force-pushed the connection-string branch from 132ce87 to 88d5717 Compare July 3, 2025 16:13
@github-actions github-actions bot removed the stale Stale due to inactivity. label Jul 4, 2025
@rbtr
Copy link
Contributor

rbtr commented Jul 8, 2025

@BeegiiK I'm not interested in maintaining a soft AppInsights fork. Have you talked to the owner of the repo to see if they would take this contribution?

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.

2 participants