-
Notifications
You must be signed in to change notification settings - Fork 57
SLCORE-1815 Feature/slcore 1647 gessie integration #1568
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
base: master
Are you sure you want to change the base?
Conversation
|
90a3595 to
06ac3ed
Compare
nquinquenel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, few comments
| LOG.info("Sending Gessie payload."); | ||
| LOG.info(json); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can probably merge the two log
| static WireMockExtension gessieEndpointMock = WireMockExtension.newInstance() | ||
| .options(wireMockConfig().dynamicPort()) | ||
| .build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we should use the dev or staging endpoint and not a mock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at this point it would make more sense to write full end-to-end tests when we will have some more relevant events to send. And also since their side still has validation problems it would just be a failing test.
| var fileContent = getTestJson("GessieRequest"); | ||
| await().untilAsserted(() -> gessieEndpointMock.verify(postRequestedFor(urlEqualTo(IDE_ENDPOINT)) | ||
| .withHeader("x-api-key", new EqualToPattern("value")) | ||
| .withRequestBody(equalToJson(fileContent)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be worth checking the actual request body with a fully formatted JSON, to be sure of what we assess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand you correctly. Do you mean to compare strictly instead of equalToJson? Wouldn't that just make the test fail when we don't actually care? I don't want it to fail when we changed field order or something.
| { | ||
| "metadata": null, | ||
| "event_payload": null | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a file for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's still test data, not the test code. And I expect to make more of these as we will develop this feature further. It wouldn't make sense to do just some of them differently.
| @PostConstruct | ||
| public void onStartup() { | ||
| if (isGessieFeatureEnabled && userSetting.isTelemetryEnabledByUser()) { | ||
| client.postEvent(new GessieEvent( | ||
| new GessieMetadata(UUID.randomUUID(), | ||
| new GessieMetadata.GessieSource(SonarLintDomain.fromProductKey(telemetryConstantAttributes.getProductKey())), | ||
| "Analytics.Editor.PluginActivated", | ||
| Long.toString(Instant.now().toEpochMilli()), | ||
| "0"), | ||
| new MessagePayload("Gessie integration test event", "slcore_start") | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a proper method that takes a GessieEvent and that handles the sending (postEvent), to have separation of concerns. Later when we have multiple event to send, they all go through this upload method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand what you mean. We already have a separate method in client: postEvent. I see here 3 things done:
- Checking if gessie is on (if content)
- building an event
- sending it via one line
client.postEvent(<event>)
What exactly you want to move? 1 & 3 into a separate method?
| private static final String GESSIE_ENDPOINT = "https://events.sonardata.io"; | ||
| private static final String IDE_SOURCE = "CiiwpdWnR21rWEOkgJ8tr3EYSXb7dzaQ5ezbipLb"; | ||
|
|
||
| @Bean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick but you can define a name with @Bean(name = xxx), just to make sure it doesn't break if someone changes the name of the method, as you prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to leave it. I like how Spring allows you to declare stuff without repeating yourself. And if somebody want to rename it they're better to do it everywhere anyway. So I would consider it a good thing that they're forced to do that.
API_CHANGES.md
Outdated
| * Introduce a new `org.sonarsource.sonarlint.core.rpc.protocol.backend.labs.IdeLabsRpcService` service and a `joinIdeLabsProgram` method. | ||
| * Use it to allow users to join the SonarQube for IDE Labs program | ||
| * The method accepts user email and IDE name as parameters | ||
| * Add a new `GESSIE_TELEMETRY` value in `org.sonarsource.sonarlint.core.rpc.protocol.backend.initialize.BackendCapability`. Clients using the feature need to declare it at initialization time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick:
- Replace by: Add a new xxx capability [...]
- From the point of view of the clients, how do they know if they should add this capability? I'd add a bit more guidance
| public SonarLintBackendBuilder withGessieTelemetryEnabled(String endpointUrl) { | ||
| this.backendCapabilities.add(GESSIE_TELEMETRY); | ||
| System.setProperty(PROPERTY_GESSIE_ENDPOINT, endpointUrl); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned above, I think we should try to rely on actual dev/staging environment and not a specific mocked endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would be able to use this part to set dev/staging later on, maybe even by default. Also then would make sense to think about speeding up retries to avoid handing tests.
| this.oldDebugValue = InternalDebug.isEnabled(); | ||
| InternalDebug.setEnabled(true); | ||
| environmentVariables.set("SONARLINT_HTTP_RETRY_INTERVAL_SECONDS", "0"); | ||
| System.setProperty(GessieSpringConfig.PROPERTY_GESSIE_API_KEY, "value"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to clear this system property after each test
| private static final String IDE_ENDPOINT = "/ide"; | ||
| public static final String FAILED_ONCE = "Failed once"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NItpick: we usually define public attributes first and then private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually meant to make it private, fixed that.
06ac3ed to
9368134
Compare
|




SLCORE-1815