Skip to content

fix: Fixing CNI Telemtry Service run by CNS #3824

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

behzad-mir
Copy link
Contributor

This PR fixes an issue where the CNI telemetry service was failing to start by CNS due to an empty Application Insights (AI) instrumentation key. The telemetry service was being initialized with a basic configuration that lacked the proper AI settings from the CNS configuration.

Requirements:

Notes:

@Copilot Copilot AI review requested due to automatic review settings July 14, 2025 16:17
@behzad-mir behzad-mir requested a review from a team as a code owner July 14, 2025 16:17
@behzad-mir behzad-mir requested a review from csfmomo July 14, 2025 16:17
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 fixes the CNI telemetry service initialization issue where it was failing to start due to an empty Application Insights instrumentation key. The fix ensures that the telemetry service uses the proper AI configuration from the CNS configuration instead of an empty basic configuration.

  • Updates the startTelemetryService function to accept CNS configuration and properly initialize AI telemetry settings
  • Adds validation to check for Application Insights instrumentation key before starting the telemetry service
  • Introduces proper telemetry configuration mapping from CNS settings to AI config

@@ -94,6 +94,7 @@ const (
// Service name.
name = "azure-cns"
pluginName = "azure-vnet"
aiPluginName = "AzureCNI"
Copy link
Preview

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

There's a typo in the PR title 'Telemtry' which should be 'Telemetry'. While this constant name is correct, ensure consistency in naming throughout the codebase.

Copilot uses AI. Check for mistakes.

@behzad-mir behzad-mir added do-not-merge cni Related to CNI. labels Jul 14, 2025
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Behzad Mirkhanzadeh <[email protected]>
@tamilmani1989 tamilmani1989 requested review from QxBytes and rbtr July 14, 2025 16:58
Comment on lines +497 to +500
if cnsconfig.TelemetrySettings.AppInsightsInstrumentationKey != "" {
err = tb.CreateAITelemetryHandle(aiConfig, ts.DisableTrace, ts.DisableMetric, ts.DisableEvent)
} else {
logger.Printf("No Application Insights key provided for CNI telemetry service")
Copy link
Member

Choose a reason for hiding this comment

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

we should check other way and remove else?

if cnsconfig.TelemetrySettings.AppInsightsInstrumentationKey == "" {
  logger.Printf("No Application Insights key provided for CNI telemetry service")
  return
}
err = tb.CreateAITelemetryHandle(aiConfig, ts.DisableTrace, ts.DisableMetric, ts.DisableEvent)

@santhoshmprabhu
Copy link
Contributor

santhoshmprabhu commented Jul 14, 2025

Discussed in the team meeting today - we may want to consider doing telemetry for CNI directly in CNI rather than CNS, to account for scenarios where CNI can't talk to CNS.
Tagging @tamilmani1989 as decided in the call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cni Related to CNI. do-not-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants