Emit chat and inline events to destination #574
Conversation
| @@ -185,6 +191,16 @@ export class TelemetryService extends CodeWhispererServiceToken { | |||
| if (options?.conversationId === undefined) { | |||
| return | |||
| } | |||
| if (this.enableTelemetryEventsToDestination) { | |||
There was a problem hiding this comment.
Let's add an unit test for this condition
There was a problem hiding this comment.
There is already a unit test update for this when I am setting telemetryService.updateTelemetryEventsToDestination to true.
| @@ -209,6 +225,18 @@ export class TelemetryService extends CodeWhispererServiceToken { | |||
| modificationPercentage: number | |||
| customizationArn?: string | |||
| }) { | |||
| if (this.enableTelemetryEventsToDestination) { | |||
There was a problem hiding this comment.
I suggest to set enableTelemetryEventsToDestination to false as default value explicitly where it is first defined first https://github.com/aws/language-servers/pull/574/files#diff-9651c5ce85ab6404bc20ff59b61b93254f540d5e677f455b75e3d17ce5c07f08R36
There was a problem hiding this comment.
I'll set as default false in the constructor itself.
pras0131
changed the title
|
As we are introducing flag, which disables telemetry sent back to destination over LSP, we need to make sure all telemetry events respect the flag value. We need to review and make sure they also are disabled on flag. |
| @@ -132,6 +140,43 @@ export class TelemetryService extends CodeWhispererServiceToken { | |||
| } | |||
|
|
|||
| public emitUserTriggerDecision(session: CodeWhispererSession, timeSinceLastUserModification?: number) { | |||
| if (this.enableTelemetryEventsToDestination) { | |||
There was a problem hiding this comment.
We still need explicit unit test for cases when enableTelemetryEventsToDestination is false. Currently we don't have test that would verify that this codepath is not called.
…etryEventsToDestination flag is disabled
| The flag enableTelemetryEventsToDestination is set to true temporarily. It's value will be determined through destination | ||
| configuration post all events migration to STE. It'll be replaced by qConfig['enableTelemetryEventsToDestination'] === true | ||
| */ | ||
| const enableTelemetryEventsToDestination = true |
There was a problem hiding this comment.
I'd disable this call completely and rely on default value set in TelemetryService class. Did it in last commit.
viktorsaws
changed the title
Problem
Reference: https://issues.amazon.com/issues/AWS-Cloud9-28780, https://issues.amazon.com/issues/AWS-Cloud9-28679
Report chat and inline completion events to destination based on the flag sent by the destination enableTelemetryEventsToDestinationUPD: in this PR we do not enable the configuration
enableTelemetryEventsToDestinationflag, and we always emit events with LSP and to SendTelemetryEvent API.We will introduce the flag to disable telemetry, when all events are migrated to SendTelemetryEvent.
Solution
All the respective chat events are moved to TelemetryService, so it being a single point to emit events to STE or destination.
The PR also removes the call where they were being called before, so they are only called based on the condition now.
For emitChatAddMessage method in telemetry service, the arguments have been divided into params and additionalParams..additionalParams are only needed for event emit to destination. So in future when emitting events to destination has to be removed, the struct can be removed all together.
There is a change in TelemetryService constructor to set the default value of enableTelemetryEventsToDestination flag to false.
Test cases in the telemetry service have been updated to make sure if the flag is on, the respective event to destination is also emitted.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.