SONARPY-2451 SONARPY-2459 SONARPY-2464 Collect data for the Jupyter notebooks #2235

ghislainpiot · 2024-12-11T15:54:40Z

joke1196

This looks quite good. I added suggestions for the shape of the API, and the workaround the functional API of java.

joke1196 · 2024-12-12T10:26:28Z

sonar-python-plugin/src/main/java/org/sonar/plugins/python/SensorTelemetryStorage.java

+
+  public SensorTelemetryStorage() {
+    data = new HashMap<>();
+    data.put(NOTEBOOK_PRESENT_KEY, "false");


I think we could hide the implementation detail of the Map by making it private and accessing it through an API that is more domain oriented, i.e. updateMetrics. Following this direction We could also provide a specific type for the metrics key, i.e. TelemetryKey so that user to not pass any string to the Telemetry but only predefined key. I think it is good to be strict here so that we do not have a mess later on when other devs try to use the telemetry API

joke1196 · 2024-12-12T10:33:09Z

sonar-python-plugin/src/main/java/org/sonar/plugins/python/IPynbSensor.java

    List<PythonInputFile> generatedIPythonFiles = new ArrayList<>();
+
+    sensorTelemetryStorage.data.put(SensorTelemetryStorage.NOTEBOOK_TOTAL_KEY, String.valueOf(pythonFiles.size()));
+    final int[] numberOfFailedParsing = {0};


I guess this is a workaround as you need a final var for the lambda? I think it would be more readable to an if

Suggested change

final int[] numberOfFailedParsing = {0};

int numberOfFailedParsing = 0;

if(result.isPresent()){

generatedIPythonFiles.add(result.get());

}else {

numberOfFailedParsing++;

}

instead of trying to work against java.

joke1196 · 2024-12-12T10:34:14Z

its/plugin/it-python-plugin-test/src/test/java/com/sonar/python/it/plugin/TestsUtils.java

@@ -50,6 +50,8 @@ public final class TestsUtils {
    .useDefaultAdminCredentialsForBuilds(true)
    .setSonarVersion(System.getProperty(SQ_VERSION_PROPERTY, DEFAULT_SQ_VERSION))
    .addPlugin(PLUGIN_LOCATION)
+    .setServerProperty("sonar.telemetry.enable", "false") // Disable telemetry waiting for ORCH-497


I don't think this should be part of this PR. Maybe a merge issue?

Just a rebase/base branch thing, should be ok now

joke1196 · 2024-12-12T13:46:15Z

sonar-python-plugin/src/main/java/org/sonar/plugins/python/SensorTelemetryStorage.java

+  public static final MetricKey NOTEBOOK_TOTAL_KEY = new MetricKey("python.notebook.total");
+  public static final MetricKey NOTEBOOK_PARSE_ERROR_KEY = new MetricKey("python.notebook.parse_error");
+  public static final MetricKey NOTEBOOK_RECOGNITION_ERROR_KEY = new MetricKey("python.notebook.recognition_error");
+  public static final MetricKey NOTEBOOK_EXCEPTION_KEY = new MetricKey("python.notebook.exceptions");
  final Map<String, String> data;


I think we could make this private

maksim-grebeniuk-sonarsource · 2024-12-16T10:25:06Z

sonar-python-plugin/src/main/java/org/sonar/plugins/python/IPynbSensor.java

  }

  private void processNotebooksFiles(List<PythonInputFile> pythonFiles, SensorContext context) {
-    pythonFiles = parseNotebooks(pythonFiles, context);
+    pythonFiles = this.parseNotebooks(pythonFiles, context);


is this this needed?

maksim-grebeniuk-sonarsource · 2024-12-16T10:30:59Z

sonar-python-plugin/src/main/java/org/sonar/plugins/python/SensorTelemetryStorage.java

+    data.put(key.key(), value);
+  }
+
+  public enum MetricKey {


Since this enum massively used outside of the SensorTelemetryStorage - probably it makes sense to make it a top-level class

maksim-grebeniuk-sonarsource · 2024-12-16T10:34:26Z

sonar-python-plugin/src/main/java/org/sonar/plugins/python/SensorTelemetryStorage.java

+public class SensorTelemetryStorage {
+  private static final Logger LOG = LoggerFactory.getLogger(SensorTelemetryStorage.class);
+
+  private final Map<String, String> data;


why the key of the data map can't be of MetricKey type?

maksim-grebeniuk-sonarsource · 2024-12-16T10:35:12Z

sonar-python-plugin/src/main/java/org/sonar/plugins/python/SensorTelemetryStorage.java

+    data = new HashMap<>();
+  }
+
+  public Map<String, String> data() {


it seems that this method is used for test purposes only, so probably it should not be public.

maksim-grebeniuk-sonarsource

LGTM, as discussed some small changes required but no need to revisit again

Seppli11

It looks good to me. However, I had a question during review and there are some minor nitpicks.

Seppli11 · 2024-12-17T13:04:13Z

sonar-python-plugin/src/main/java/org/sonar/plugins/python/IPynbSensor.java

    for (PythonInputFile inputFile : pythonFiles) {
      try {
+        sensorTelemetryStorage.updateMetric(TelemetryMetricKey.NOTEBOOK_PRESENT_KEY, "1");


what happens when the key is not present?

The field is just not present. I don't think it is possible to have a default value

That doesn't pose an issue to analyze the data afterward?

I don't think it will, we cannot set a default value because if there are no notebooks the sensor doesn't run

Seppli11 · 2024-12-17T13:05:32Z

sonar-python-plugin/src/main/java/org/sonar/plugins/python/SensorTelemetryStorage.java

+    }
+  }
+
+  public void updateMetric(TelemetryMetricKey key, String value) {


There are a few calls to updateMetric where value is an int and String.valueOf is called. Would it maybe make sense to have an overload?

Seppli11 · 2024-12-17T13:06:58Z

sonar-python-plugin/src/main/java/org/sonar/plugins/python/SensorTelemetryStorage.java

+  private final Map<TelemetryMetricKey, String> data;
+
+  public SensorTelemetryStorage() {
+    data = new EnumMap<>(TelemetryMetricKey.class);


nitpick: the assignment could be moved to the field declaration and the constructor can be removed

sonarqube-next · 2024-12-17T14:39:59Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

Seppli11

LGTM

ghislainpiot force-pushed the SONARPY-2451 branch from 6955f08 to 590f311 Compare December 11, 2024 16:31

ghislainpiot requested a review from joke1196 December 11, 2024 18:23

ghislainpiot force-pushed the SONARPY-2451 branch from 590f311 to 6d742a8 Compare December 12, 2024 08:35

ghislainpiot changed the base branch from master to SONARPY-2454 December 12, 2024 08:35

Base automatically changed from SONARPY-2454 to master December 12, 2024 08:43

joke1196 requested changes Dec 12, 2024

View reviewed changes

ghislainpiot force-pushed the SONARPY-2451 branch 2 times, most recently from ebb485b to fa62a77 Compare December 12, 2024 13:17

joke1196 changed the title ~~SONARPY-2451 Collect data for the Jupyter notebooks~~ SONARPY-2451 SONARPY-2459 Collect data for the Jupyter notebooks Dec 12, 2024

joke1196 reviewed Dec 12, 2024

View reviewed changes

joke1196 approved these changes Dec 12, 2024

View reviewed changes

ghislainpiot requested a review from joke1196 December 12, 2024 14:36

ghislainpiot force-pushed the SONARPY-2451 branch 2 times, most recently from 5595ef4 to 98205bc Compare December 13, 2024 16:24

ghislainpiot changed the title ~~SONARPY-2451 SONARPY-2459 Collect data for the Jupyter notebooks~~ SONARPY-2451 SONARPY-2459 SONARPY-2464 Collect data for the Jupyter notebooks Dec 13, 2024

ghislainpiot force-pushed the SONARPY-2451 branch from 98205bc to 67270df Compare December 16, 2024 08:37

ghislainpiot requested a review from maksim-grebeniuk-sonarsource December 16, 2024 09:57

ghislainpiot force-pushed the SONARPY-2451 branch from 67270df to 3823b50 Compare December 16, 2024 10:21

maksim-grebeniuk-sonarsource reviewed Dec 16, 2024

View reviewed changes

maksim-grebeniuk-sonarsource approved these changes Dec 16, 2024

View reviewed changes

ghislainpiot force-pushed the SONARPY-2451 branch 3 times, most recently from bf32141 to d7f764c Compare December 16, 2024 13:06

SONARPY-2459 Change organization name in pom.xml

869bc4a

ghislainpiot force-pushed the SONARPY-2451 branch 2 times, most recently from cba6ca6 to 47eab69 Compare December 17, 2024 10:18

ghislainpiot requested a review from Seppli11 December 17, 2024 11:03

Seppli11 requested changes Dec 17, 2024

View reviewed changes

ghislainpiot added 2 commits December 17, 2024 15:32

SONARPY-2451 Collect data for the Jupyter notebooks

2bff199

SONARPY-2464 Add safety around the telemetry mechanism

cddb775

ghislainpiot force-pushed the SONARPY-2451 branch from 47eab69 to cddb775 Compare December 17, 2024 14:33

Seppli11 approved these changes Dec 17, 2024

View reviewed changes

ghislainpiot merged commit a22456e into master Dec 17, 2024
13 checks passed

ghislainpiot deleted the SONARPY-2451 branch December 17, 2024 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SONARPY-2451 SONARPY-2459 SONARPY-2464 Collect data for the Jupyter notebooks #2235

SONARPY-2451 SONARPY-2459 SONARPY-2464 Collect data for the Jupyter notebooks #2235

ghislainpiot commented Dec 11, 2024 •

edited

Loading

joke1196 left a comment

joke1196 Dec 12, 2024

joke1196 Dec 12, 2024

joke1196 Dec 12, 2024

ghislainpiot Dec 12, 2024

joke1196 Dec 12, 2024

maksim-grebeniuk-sonarsource Dec 16, 2024

maksim-grebeniuk-sonarsource Dec 16, 2024

maksim-grebeniuk-sonarsource Dec 16, 2024

maksim-grebeniuk-sonarsource Dec 16, 2024

maksim-grebeniuk-sonarsource left a comment

Seppli11 left a comment

Seppli11 Dec 17, 2024

ghislainpiot Dec 17, 2024

Seppli11 Dec 17, 2024

ghislainpiot Dec 17, 2024

Seppli11 Dec 17, 2024

Seppli11 Dec 17, 2024

sonarqube-next bot commented Dec 17, 2024

Seppli11 left a comment

-    final int[] numberOfFailedParsing = {0};
+int numberOfFailedParsing = 0;
+if(result.isPresent()){
+  generatedIPythonFiles.add(result.get());
+}else {
+  numberOfFailedParsing++;
+}

SONARPY-2451 SONARPY-2459 SONARPY-2464 Collect data for the Jupyter notebooks #2235

SONARPY-2451 SONARPY-2459 SONARPY-2464 Collect data for the Jupyter notebooks #2235

Conversation

ghislainpiot commented Dec 11, 2024 • edited Loading

joke1196 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maksim-grebeniuk-sonarsource left a comment

Choose a reason for hiding this comment

Seppli11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqube-next bot commented Dec 17, 2024

Quality Gate passed

Seppli11 left a comment

Choose a reason for hiding this comment

ghislainpiot commented Dec 11, 2024 •

edited

Loading