feat: Add telemetry to espressif-ide #1082
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds telemetry functionality to the Espressif IDE using Azure Application Insights. The implementation includes the necessary infrastructure to track user events and usage patterns within the IDE.
- Integrates Azure Application Insights telemetry agent and SDK
- Adds telemetry logging capability with a new TelemetryLogger class
- Implements initial event tracking for new project creation
Reviewed Changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| releng/com.espressif.idf.product/idf.product | Adds Java agent configuration for telemetry across all platforms |
| features/com.espressif.idf.feature/feature.xml | Includes the new telemetry plugin in the feature definition |
| bundles/pom.xml | Adds telemetry module to the build configuration |
| bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/wizard/NewIDFProjectWizard.java | Implements telemetry event logging for new project creation |
| bundles/com.espressif.idf.telemetry/* | New telemetry bundle with configuration files and agent JAR |
| bundles/com.espressif.idf.core/* | Adds TelemetryLogger class and Application Insights SDK dependency |
| @@ -0,0 +1,6 @@ | |||
| { | |||
| "connectionString": "InstrumentationKey=912cbb4e-e606-4c4f-9a00-1731acb15117;IngestionEndpoint=https://eastasia-0.in.applicationinsights.azure.com/;LiveEndpoint=https://eastasia.livediagnostics.monitor.azure.com/;ApplicationId=1efdbec9-ed3f-4e75-a44d-67c6708b6d79", | |||
There was a problem hiding this comment.
The connection string contains sensitive credentials (InstrumentationKey and ApplicationId) that are hardcoded in the configuration file. These credentials should be externalized to environment variables or a secure configuration management system to prevent exposure in version control.
| "connectionString": "InstrumentationKey=912cbb4e-e606-4c4f-9a00-1731acb15117;IngestionEndpoint=https://eastasia-0.in.applicationinsights.azure.com/;LiveEndpoint=https://eastasia.livediagnostics.monitor.azure.com/;ApplicationId=1efdbec9-ed3f-4e75-a44d-67c6708b6d79", | |
| "connectionString": "InstrumentationKey=${INSTRUMENTATION_KEY};IngestionEndpoint=https://eastasia-0.in.applicationinsights.azure.com/;LiveEndpoint=https://eastasia.livediagnostics.monitor.azure.com/;ApplicationId=${APPLICATION_ID}", |
| updateClangFiles(project); | ||
|
|
||
| //update telemetry | ||
| Map<String, String> properties = new HashMap<String, String>(); |
There was a problem hiding this comment.
[nitpick] Use diamond operator for better code readability: new HashMap<>() instead of new HashMap<String, String>().
| Map<String, String> properties = new HashMap<String, String>(); | |
| Map<String, String> properties = new HashMap<>(); |
| public static void logEvent(String eventName, Map<String, String> properties) | ||
| { | ||
| telemetryClient.trackEvent(eventName, properties, null); | ||
| telemetryClient.flush(); |
There was a problem hiding this comment.
Calling flush() after every event can impact performance as it forces immediate transmission. Consider batching events or using async transmission, especially if this method will be called frequently.
| telemetryClient.flush(); | |
| // Removed flush() to allow batching and improve performance. |
| private static final TelemetryClient telemetryClient = new TelemetryClient(); | ||
|
|
||
| public static void logEvent(String eventName, Map<String, String> properties) | ||
| { | ||
| telemetryClient.trackEvent(eventName, properties, null); | ||
| telemetryClient.flush(); |
There was a problem hiding this comment.
The TelemetryClient is created without any configuration or error handling. Consider adding initialization logic to handle cases where telemetry might not be available or configured properly.
| private static final TelemetryClient telemetryClient = new TelemetryClient(); | |
| public static void logEvent(String eventName, Map<String, String> properties) | |
| { | |
| telemetryClient.trackEvent(eventName, properties, null); | |
| telemetryClient.flush(); | |
| private static final TelemetryClient telemetryClient; | |
| static { | |
| TelemetryClient client = null; | |
| try { | |
| // Try to get connection string or instrumentation key from environment or system properties | |
| String connectionString = System.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"); | |
| if (connectionString == null || connectionString.isEmpty()) { | |
| connectionString = System.getProperty("APPLICATIONINSIGHTS_CONNECTION_STRING"); | |
| } | |
| if (connectionString != null && !connectionString.isEmpty()) { | |
| TelemetryConfiguration configuration = TelemetryConfiguration.getActive(); | |
| configuration.setConnectionString(connectionString); | |
| client = new TelemetryClient(configuration); | |
| } else { | |
| // Optionally, try instrumentation key for legacy support | |
| String instrumentationKey = System.getenv("APPINSIGHTS_INSTRUMENTATIONKEY"); | |
| if (instrumentationKey == null || instrumentationKey.isEmpty()) { | |
| instrumentationKey = System.getProperty("APPINSIGHTS_INSTRUMENTATIONKEY"); | |
| } | |
| if (instrumentationKey != null && !instrumentationKey.isEmpty()) { | |
| TelemetryConfiguration configuration = TelemetryConfiguration.getActive(); | |
| configuration.setInstrumentationKey(instrumentationKey); | |
| client = new TelemetryClient(configuration); | |
| } else { | |
| System.err.println("TelemetryLogger: No Application Insights connection string or instrumentation key found. Telemetry is disabled."); | |
| } | |
| } | |
| } catch (Exception e) { | |
| System.err.println("TelemetryLogger: Failed to initialize TelemetryClient: " + e.getMessage()); | |
| } | |
| telemetryClient = client; | |
| } | |
| public static void logEvent(String eventName, Map<String, String> properties) | |
| { | |
| if (telemetryClient == null) { | |
| // Telemetry is not configured; optionally log or ignore | |
| return; | |
| } | |
| try { | |
| telemetryClient.trackEvent(eventName, properties, null); | |
| telemetryClient.flush(); | |
| } catch (Exception e) { | |
| System.err.println("TelemetryLogger: Failed to log telemetry event: " + e.getMessage()); | |
| } |
| import com.espressif.idf.core.logging.TelemetryLogger; | ||
| import com.espressif.idf.core.util.ClangFormatFileHandler; | ||
| import com.espressif.idf.core.util.ClangdConfigFileHandler; | ||
| import com.espressif.idf.core.util.EclipseIniUtil; |
There was a problem hiding this comment.
The import for EclipseIniUtil is added but not used in the visible code changes. This unused import should be removed unless it's used elsewhere in the method.
| import com.espressif.idf.core.util.EclipseIniUtil; |
Description
Please include a summary of the change and which issue is fixed.
Fixes # (IEP-XXX)
Type of change
Please delete options that are not relevant.
How has this been tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Dependent components impacted by this PR:
Checklist