Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SONARPY-2457 Collect data for the Python version #2252

Merged
merged 2 commits into from
Dec 17, 2024
Merged

SONARPY-2457 Collect data for the Python version #2252

merged 2 commits into from
Dec 17, 2024

Conversation

ghislainpiot
Copy link
Contributor

@ghislainpiot ghislainpiot commented Dec 16, 2024 •
edited by jira bot
Loading

@ghislainpiot ghislainpiot force-pushed the SONARPY-2451 branch 2 times, most recently from a5e68b5 to bf32141 Compare December 16, 2024 11:04
@ghislainpiot ghislainpiot force-pushed the SONARPY-2457 branch 2 times, most recently from 2e3fc3d to 832ad9d Compare December 16, 2024 11:06
@ghislainpiot ghislainpiot force-pushed the SONARPY-2457 branch 2 times, most recently from 9f01657 to c7c3bc3 Compare December 16, 2024 13:15
@ghislainpiot ghislainpiot force-pushed the SONARPY-2457 branch 3 times, most recently from 26eebf3 to 91ec907 Compare December 16, 2024 15:15
guillaume-dequenne-sonarsource
guillaume-dequenne-sonarsource reviewed Dec 16, 2024
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm missing some context on this topic, so I might miss things, and I'm taking what was already implemented as reference.

This looks good though! I left some questions for my own comprehension. My only concern is about the strategy for the isVersionSet key that I could see skewed when counting SonarLint.

sonar-python-plugin/src/main/java/org/sonar/plugins/python/PythonSensor.java Outdated
durationReport.stop();
}

private void setTelemetry(SensorContext context, String[] pythonVersionParameter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the method seems a bit misleading I guess, what we're doing here is update the Python version telemetry rather than "setting it", no? (I initially thought this was about setting a telemetry field to the context...)
What about updatePythonVersionTelemetry ?

sonar-python-plugin/src/main/java/org/sonar/plugins/python/PythonSensor.java Outdated

public PythonSensor(FileLinesContextFactory fileLinesContextFactory, CheckFactory checkFactory, NoSonarFilter noSonarFilter,
@Nullable PythonCustomRuleRepository[] customRuleRepositories, @Nullable PythonIndexer indexer,
@Nullable SonarLintCache sonarLintCache, AnalysisWarningsWrapper analysisWarnings, SensorTelemetryStorage sensorTelemetryStorage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand in which case we should expect this sensorTelemetryStorage to be injected and in which case we should expect to instantiate it ourselves, and what difference it makes, can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A similar point was raised by Maksim. The goal is to be able to inject the storage as a mock and test it that way.
I should probably rewrite it to use Mockito on the sendTelemetryProperty mock on the SensorContextTester

sonar-python-plugin/src/main/java/org/sonar/plugins/python/PythonSensor.java Outdated
durationReport.stop();
}

private void setTelemetry(SensorContext context, String[] pythonVersionParameter) {
var isVersionSet = pythonVersionParameter.length != 0 || context.runtime().getProduct() == SonarProduct.SONARLINT;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we consider the version to be set in SonarLint context? Wouldn't that skew the data and imply more people actively set the version than what actually happens?

sonar-python-plugin/src/main/java/org/sonar/plugins/python/PythonSensor.java
private void setTelemetry(SensorContext context, String[] pythonVersionParameter) {
var isVersionSet = pythonVersionParameter.length != 0 || context.runtime().getProduct() == SonarProduct.SONARLINT;
if (pythonVersionParameter.length != 0) {
sensorTelemetryStorage.updateMetric(TelemetryMetricKey.PYTHON_VERSION_KEY, String.join(",", pythonVersionParameter));
Copy link
Contributor

Choose a reason for hiding this comment

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

So that means we can set more than just booleans for the telemetry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, CFamily is sending strings.

Seppli11
Seppli11 approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@Seppli11 Seppli11 left a comment

Choose a reason for hiding this comment

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

LGTM!
Out of curiosity, why don't we send telemetry from sonarlint? Would it just screw up the data too much?

@ghislainpiot
Copy link
Contributor Author

LGTM! Out of curiosity, why don't we send telemetry from sonarlint? Would it just screw up the data too much?

I don't think telemetry is sent right now on their side anyways.
It would also probably screw up the stats

@ghislainpiot ghislainpiot force-pushed the SONARPY-2457 branch 2 times, most recently from 63aa67f to 0c34b58 Compare December 17, 2024 14:47
Base automatically changed from SONARPY-2451 to master December 17, 2024 14:52
@ghislainpiot ghislainpiot enabled auto-merge (squash) December 17, 2024 16:12
@sonarqube-next SonarQube-Next
Copy link

Quality Gate passed 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

@ghislainpiot ghislainpiot merged commit fc085d2 into master Dec 17, 2024
10 checks passed
@ghislainpiot ghislainpiot deleted the SONARPY-2457 branch December 17, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guillaume-dequenne-sonarsource guillaume-dequenne-sonarsource guillaume-dequenne-sonarsource left review comments

@Seppli11 Seppli11 Seppli11 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ghislainpiot @Seppli11 @guillaume-dequenne-sonarsource