-
Notifications
You must be signed in to change notification settings - Fork 250
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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)
987145b
to
fa88a0b
Compare
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 |
132ce87
to
88d5717
Compare
@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? |
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 moduleIssue Fixed:
#3730
Requirements:
Notes: