feat: add anonymous usage telemetry for Espressif-IDE - #1494
Merged
Conversation
Report install, update and daily session events to Azure Application Insights so we can count active users and release adoption, with installation-wide opt-out, a first-run notice and docs.
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryPreferences.java`:
- Around line 162-181: Update TelemetryPreferences.getStateNode and flush so
configuration-scope state remains the primary read/write location, but a
BackingStoreException during configuration flush retries persistence in the
InstanceScope node. Add the corresponding read fallback so values written to
InstanceScope are used when ConfigurationScope is unavailable or read-only,
preserving NOTICE_SHOWN, telemetryInstallId, telemetryLastReportedVersion, and
telemetryLastSessionReport across restarts; cover the read-only configuration
case.
- Around line 63-68: Update isDisabledByOverride() to evaluate both opt-out
sources: check the system property first, but only return true when it contains
a disabling value; otherwise continue checking TELEMETRY_ENV_VARIABLE and return
true when it is disabled. Preserve the existing blank-value fallback and
trimmed-value handling.
In `@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/TelemetryNotice.java`:
- Line 36: Update TelemetryNotice’s documentation-link handling to select the
URL for the active locale instead of always using the English `/en/latest/`
path. Reuse the localized message/resource mechanism associated with
messages_zh.properties, and ensure the selected URL is used when opening the
browser.
- Around line 49-73: Update TelemetryNotice.showIfNeeded and open so
TelemetryPreferences.setNoticeShown is called only after NotificationPopup
successfully opens. Remove the eager state update before display.asyncExec, and
ensure the disposed-display guard in open leaves the notice unmarked so a later
session can show it.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae99cdc4-54e3-4204-90be-019714938254
📒 Files selected for processing (25)
bundles/com.espressif.idf.core/META-INF/MANIFEST.MFbundles/com.espressif.idf.core/src/com/espressif/idf/core/IDFCorePreferenceConstants.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryConnection.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryEnvelope.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryPreferences.javabundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryService.javabundles/com.espressif.idf.ui/META-INF/MANIFEST.MFbundles/com.espressif.idf.ui/plugin.xmlbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/Messages.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/TelemetryNotice.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/TelemetryStartup.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/messages.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/messages_zh.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/Messages.javabundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages.propertiesbundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/messages_zh.propertiesdocs/en/index.rstdocs/en/telemetry.rstdocs/zh_CN/index.rstdocs/zh_CN/telemetry.rsttests/com.espressif.idf.core.test/META-INF/MANIFEST.MFtests/com.espressif.idf.core.test/src/com/espressif/idf/core/telemetry/test/TelemetryConnectionTest.javatests/com.espressif.idf.core.test/src/com/espressif/idf/core/telemetry/test/TelemetryEnvelopeTest.javatests/com.espressif.idf.core.test/src/com/espressif/idf/core/telemetry/test/TelemetrySessionIntervalTest.java
Comment on lines
+63
to
+68
| String property = System.getProperty(TELEMETRY_SYSTEM_PROPERTY); | ||
| if (property == null || property.isBlank()) | ||
| { | ||
| property = System.getenv(TELEMETRY_ENV_VARIABLE); | ||
| } | ||
| return property != null && isDisabledValue(property.trim()); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="bundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryPreferences.java"
echo "== candidate file lines =="
sed -n '1,220p' "$file" | nl -ba
echo "== telemetry constants/usages =="
rg -n "TELEMETRY_SYSTEM_PROPERTY|TELEMETRY_ENV_VARIABLE|isDisabledByOverride|IDF_TELEMETRY|idf\.telemetry|Enabled\(|isEnabled\(" -S .
echo "== behavioral probe: current isDisabledByOverride logic =="
python3 - <<'PY'
def current(property_value, env_value):
property = property_value
if property == None or property.strip() == "":
property = env_value
value = property.strip() if property else None
return value is not None and value.lower() in ("0", "false", "no")
cases = [
("true", "0"),
("", "0"),
(None, "0"),
("1", "0"),
("0", "1"),
("true", None),
]
for prop, env in cases:
print(repr(prop), repr(env), "=>", current(prop, env))
PYRepository: espressif/idf-eclipse-plugin
Length of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="bundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryPreferences.java"
echo "== candidate file lines =="
awk '{printf "%s\t%s\n", NR, $0}' "$file" | sed -n '1,220p'
echo
echo "== telemetry constants/usages =="
rg -n "TELEMETRY_SYSTEM_PROPERTY|TELEMETRY_ENV_VARIABLE|isDisabledByOverride|IDF_TELEMETRY|idf\.telemetry|Enabled\(|isEnabled\(" -S . 2>/dev/null || true
echo
echo "== behavioral probe: current isDisabledByOverride logic =="
python3 - <<'PY'
def current(property_value, env_value):
property = property_value
if property == None or property.strip() == "":
property = env_value
value = property.strip() if property else None
return value is not None and value.lower() in ("0", "false", "no")
cases = [
("true", "0"),
("", "0"),
(None, "0"),
("1", "0"),
("0", "1"),
("true", None),
]
for prop, env in cases:
print(repr(prop), repr(env), "=>", current(prop, env))
PYRepository: espressif/idf-eclipse-plugin
Length of output: 29806
Sensitive Data Exposure (CWE-359)
Reachability path
● Entry
bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/preferences/EspresssifPreferencesPage.java:98
addPrivacySettings
│
▼
● Sink
bundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryPreferences.java
Evaluate both opt-out sources.
A nonblank enabling system property currently prevents checking IDF_TELEMETRY=0, so isDisabledByOverride() returns false and telemetry can run despite the environment variable opt-out. When the system property is not disabling, continue and apply the environment-variable opt-out.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryPreferences.java`
around lines 63 - 68, Update isDisabledByOverride() to evaluate both opt-out
sources: check the system property first, but only return true when it contains
a disabling value; otherwise continue checking TELEMETRY_ENV_VARIABLE and return
true when it is disabled. Preserve the existing blank-value fallback and
trimmed-value handling.
Comment on lines
+162
to
+181
| private static IEclipsePreferences getStateNode() | ||
| { | ||
| IEclipsePreferences node = ConfigurationScope.INSTANCE.getNode(IDFCorePlugin.PLUGIN_ID); | ||
| if (node == null) | ||
| { | ||
| node = InstanceScope.INSTANCE.getNode(IDFCorePlugin.PLUGIN_ID); | ||
| } | ||
| return node; | ||
| } | ||
|
|
||
| private static void flush(IEclipsePreferences node) | ||
| { | ||
| try | ||
| { | ||
| node.flush(); | ||
| } | ||
| catch (BackingStoreException e) | ||
| { | ||
| // A read-only configuration area only means the state is recomputed on the next start | ||
| Logger.log(e, true); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and relevant symbols"
if [ -f bundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryPreferences.java ]; then
wc -l bundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryPreferences.java
sed -n '1,260p' bundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryPreferences.java
else
echo "File not found"
fd -i 'TelemetryPreferences.java' .
fi
echo
echo "Search telemetry persistence usages"
rg -n "TelemetryPreferences|getStateNode|flush\\(|ConfigurationScope|InstanceScope|noticeFlag|installationId|reportedVersion|sessionTimestamp" bundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry -SRepository: espressif/idf-eclipse-plugin
Length of output: 11313
Persist installation telemetry in InstanceScope when configuration scope is read-only.
getStateNode() selects ConfigurationScope whenever getNode() succeeds, and flush() only logs BackingStoreException. If that write fails, NOTICE_SHOWN, telemetryInstallId, telemetryLastReportedVersion, and telemetryLastSessionReport are not saved and can reset on restart. Add a read/read-fallback plus write fallback to InstanceScope after configuration-scope flush failure, and cover the read-only configuration area.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@bundles/com.espressif.idf.core/src/com/espressif/idf/core/telemetry/TelemetryPreferences.java`
around lines 162 - 181, Update TelemetryPreferences.getStateNode and flush so
configuration-scope state remains the primary read/write location, but a
BackingStoreException during configuration flush retries persistence in the
InstanceScope node. Add the corresponding read fallback so values written to
InstanceScope are used when ConfigurationScope is unavailable or read-only,
preserving NOTICE_SHOWN, telemetryInstallId, telemetryLastReportedVersion, and
telemetryLastSessionReport across restarts; cover the read-only configuration
case.
| { | ||
| private static final String LEARN_MORE_HREF = "learnMore"; //$NON-NLS-1$ | ||
| private static final String DISABLE_HREF = "disable"; //$NON-NLS-1$ | ||
| private static final String DOCUMENTATION_URL = "https://docs.espressif.com/projects/espressif-ide/en/latest/telemetry.html"; //$NON-NLS-1$ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Select the documentation URL from the active locale.
The Chinese notice uses messages_zh.properties, but DOCUMENTATION_URL always opens /en/latest/. Select the localized documentation URL before opening the browser.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/TelemetryNotice.java`
at line 36, Update TelemetryNotice’s documentation-link handling to select the
URL for the active locale instead of always using the English `/en/latest/`
path. Reuse the localized message/resource mechanism associated with
messages_zh.properties, and ensure the selected URL is used when opening the
browser.
Comment on lines
+49
to
+73
| public static void showIfNeeded() | ||
| { | ||
| if (TelemetryPreferences.isNoticeShown() || !TelemetryService.getInstance().isEnabled() | ||
| || !PlatformUI.isWorkbenchRunning()) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| Display display = PlatformUI.getWorkbench().getDisplay(); | ||
| if (display.isDisposed()) | ||
| { | ||
| return; | ||
| } | ||
| TelemetryPreferences.setNoticeShown(); | ||
| display.asyncExec(() -> open(display)); | ||
| } | ||
|
|
||
| private static void open(Display display) | ||
| { | ||
| if (display.isDisposed()) | ||
| { | ||
| return; | ||
| } | ||
| NotificationPopup.forDisplay(display).title(Messages.TelemetryNotice_Title, true) | ||
| .content(TelemetryNotice::createContent).delay(CLOSE_DELAY_MS).open(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Persist the notice state after the popup opens.
Line 62 marks the notice as shown before the queued UI task runs. If the display is disposed before open(display), Lines 68-71 return and later sessions never show the notice. Set the state only after the popup opens successfully.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/TelemetryNotice.java`
around lines 49 - 73, Update TelemetryNotice.showIfNeeded and open so
TelemetryPreferences.setNoticeShown is called only after NotificationPopup
successfully opens. Remove the eager state update before display.asyncExec, and
ensure the disposed-display guard in open leaves the notice unmarked so a later
session can show it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Report install, update and daily session events to Azure Application Insights so we can count active users and release adoption, with installation-wide opt-out, a first-run notice and docs.
Preferences
Notification
Description
Please include a summary of the change and which issue is fixed.
Fixes # (IEP-XXX)
Type of change
How has this been tested?
Test Configuration:
Dependent components impacted by this PR:
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests