chore(cli): updates to telemetry sink classes (#696)

Follow up to #585

---
By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license

Author: kaizencc

packages/aws-cdk/lib/cli/telemetry/endpoint-sink.ts

@@ -1,7 +1,7 @@
 import type { IncomingMessage } from 'http';
 import type { Agent } from 'https';
 import { request } from 'https';
-import type { UrlWithStringQuery } from 'url';
+import { parse, type UrlWithStringQuery } from 'url';
 import { ToolkitError } from '@aws-cdk/toolkit-lib';
 import { IoHelper } from '../../api-private';
 import type { IIoHost } from '../io-host';
@@ -17,7 +17,7 @@ export interface EndpointTelemetrySinkProps {
   /**
    * The external endpoint to hit
    */
-  readonly endpoint: UrlWithStringQuery;
+  readonly endpoint: string;
 
   /**
    * Where messages are going to be sent
@@ -44,7 +44,7 @@ export class EndpointTelemetrySink implements ITelemetrySink {
   private agent?: Agent;
 
   public constructor(props: EndpointTelemetrySinkProps) {
-    this.endpoint = props.endpoint;
+    this.endpoint = parse(props.endpoint);
     this.ioHelper = IoHelper.fromActionAwareIoHost(props.ioHost);
     this.agent = props.agent;
 
@@ -70,7 +70,7 @@ export class EndpointTelemetrySink implements ITelemetrySink {
         return;
       }
 
-      const res = await this.https(this.endpoint, this.events);
+      const res = await this.https(this.endpoint, { events: this.events });
 
       // Clear the events array after successful output
       if (res) {
@@ -87,7 +87,7 @@ export class EndpointTelemetrySink implements ITelemetrySink {
    */
   private async https(
     url: UrlWithStringQuery,
-    body: TelemetrySchema[],
+    body: { events: TelemetrySchema[] },
   ): Promise<boolean> {
     try {
       const res = await doRequest(url, body, this.agent);
@@ -112,7 +112,7 @@ export class EndpointTelemetrySink implements ITelemetrySink {
  */
 function doRequest(
   url: UrlWithStringQuery,
-  data: TelemetrySchema[],
+  data: { events: TelemetrySchema[] },
   agent?: Agent,
 ) {
   return new Promise<IncomingMessage>((ok, ko) => {

packages/aws-cdk/test/cli/telemetry/endpoint-sink.test.ts

@@ -1,5 +1,4 @@
 import * as https from 'https';
-import { parse } from 'url';
 import { IoHelper } from '../../../lib/api-private';
 import { CliIoHost } from '../../../lib/cli/io-host';
 import { EndpointTelemetrySink } from '../../../lib/cli/telemetry/endpoint-sink';
@@ -88,16 +87,15 @@ describe('EndpointTelemetrySink', () => {
   test('makes a POST request to the specified endpoint', async () => {
     // GIVEN
     const mockRequest = setupMockRequest();
-    const endpoint = parse('https://example.com/telemetry');
     const testEvent = createTestEvent('test', { foo: 'bar' });
-    const client = new EndpointTelemetrySink({ endpoint, ioHost });
+    const client = new EndpointTelemetrySink({ endpoint: 'https://example.com/telemetry', ioHost });
 
     // WHEN
     await client.emit(testEvent);
     await client.flush();
 
     // THEN
-    const expectedPayload = JSON.stringify([testEvent]);
+    const expectedPayload = JSON.stringify({ events: [testEvent] });
     expect(https.request).toHaveBeenCalledWith({
       hostname: 'example.com',
       port: null,
@@ -117,9 +115,8 @@ describe('EndpointTelemetrySink', () => {
   test('silently catches request errors', async () => {
     // GIVEN
     const mockRequest = setupMockRequest();
-    const endpoint = parse('https://example.com/telemetry');
     const testEvent = createTestEvent('test');
-    const client = new EndpointTelemetrySink({ endpoint, ioHost });
+    const client = new EndpointTelemetrySink({ endpoint: 'https://example.com/telemetry', ioHost });
 
     mockRequest.on.mockImplementation((event, callback) => {
       if (event === 'error') {
@@ -137,18 +134,17 @@ describe('EndpointTelemetrySink', () => {
   test('multiple events sent as one', async () => {
     // GIVEN
     const mockRequest = setupMockRequest();
-    const endpoint = parse('https://example.com/telemetry');
     const testEvent1 = createTestEvent('test1', { foo: 'bar' });
     const testEvent2 = createTestEvent('test2', { foo: 'bazoo' });
-    const client = new EndpointTelemetrySink({ endpoint, ioHost });
+    const client = new EndpointTelemetrySink({ endpoint: 'https://example.com/telemetry', ioHost });
 
     // WHEN
     await client.emit(testEvent1);
     await client.emit(testEvent2);
     await client.flush();
 
     // THEN
-    const expectedPayload = JSON.stringify([testEvent1, testEvent2]);
+    const expectedPayload = JSON.stringify({ events: [testEvent1, testEvent2] });
     expect(https.request).toHaveBeenCalledTimes(1);
     expect(https.request).toHaveBeenCalledWith({
       hostname: 'example.com',
@@ -169,10 +165,9 @@ describe('EndpointTelemetrySink', () => {
   test('successful flush clears events cache', async () => {
     // GIVEN
     setupMockRequest();
-    const endpoint = parse('https://example.com/telemetry');
     const testEvent1 = createTestEvent('test1', { foo: 'bar' });
     const testEvent2 = createTestEvent('test2', { foo: 'bazoo' });
-    const client = new EndpointTelemetrySink({ endpoint, ioHost });
+    const client = new EndpointTelemetrySink({ endpoint: 'https://example.com/telemetry', ioHost });
 
     // WHEN
     await client.emit(testEvent1);
@@ -181,7 +176,7 @@ describe('EndpointTelemetrySink', () => {
     await client.flush();
 
     // THEN
-    const expectedPayload1 = JSON.stringify([testEvent1]);
+    const expectedPayload1 = JSON.stringify({ events: [testEvent1] });
     expect(https.request).toHaveBeenCalledTimes(2);
     expect(https.request).toHaveBeenCalledWith({
       hostname: 'example.com',
@@ -196,7 +191,7 @@ describe('EndpointTelemetrySink', () => {
       timeout: 500,
     }, expect.anything());
 
-    const expectedPayload2 = JSON.stringify([testEvent2]);
+    const expectedPayload2 = JSON.stringify({ events: [testEvent2] });
     expect(https.request).toHaveBeenCalledWith({
       hostname: 'example.com',
       port: null,
@@ -238,10 +233,9 @@ describe('EndpointTelemetrySink', () => {
       return mockRequest;
     });
 
-    const endpoint = parse('https://example.com/telemetry');
     const testEvent1 = createTestEvent('test1', { foo: 'bar' });
     const testEvent2 = createTestEvent('test2', { foo: 'bazoo' });
-    const client = new EndpointTelemetrySink({ endpoint, ioHost });
+    const client = new EndpointTelemetrySink({ endpoint: 'https://example.com/telemetry', ioHost });
 
     // WHEN
     await client.emit(testEvent1);
@@ -255,7 +249,7 @@ describe('EndpointTelemetrySink', () => {
     await client.flush();
 
     // THEN
-    const expectedPayload1 = JSON.stringify([testEvent1]);
+    const expectedPayload1 = JSON.stringify({ events: [testEvent1] });
     expect(https.request).toHaveBeenCalledTimes(2);
     expect(https.request).toHaveBeenCalledWith({
       hostname: 'example.com',
@@ -270,15 +264,15 @@ describe('EndpointTelemetrySink', () => {
       timeout: 500,
     }, expect.anything());
 
-    const expectedPayload2 = JSON.stringify([testEvent2]);
+    const expectedPayload2 = JSON.stringify({ events: [testEvent1, testEvent2] });
     expect(https.request).toHaveBeenCalledWith({
       hostname: 'example.com',
       port: null,
       path: '/telemetry',
       method: 'POST',
       headers: {
         'content-type': 'application/json',
-        'content-length': expectedPayload1.length + expectedPayload2.length - 1,
+        'content-length': expectedPayload2.length,
       },
       agent: undefined,
       timeout: 500,
@@ -289,13 +283,12 @@ describe('EndpointTelemetrySink', () => {
     // GIVEN
     jest.useFakeTimers();
     setupMockRequest(); // Setup the mock request but we don't need the return value
-    const endpoint = parse('https://example.com/telemetry');
 
     // Create a spy on setInterval
     const setIntervalSpy = jest.spyOn(global, 'setInterval');
 
     // Create the client
-    const client = new EndpointTelemetrySink({ endpoint, ioHost });
+    const client = new EndpointTelemetrySink({ endpoint: 'https://example.com/telemetry', ioHost });
 
     // Create a spy on the flush method
     const flushSpy = jest.spyOn(client, 'flush');
@@ -337,8 +330,7 @@ describe('EndpointTelemetrySink', () => {
     // Mock IoHelper.fromActionAwareIoHost to return our mock
     jest.spyOn(IoHelper, 'fromActionAwareIoHost').mockReturnValue(mockIoHelper as any);
 
-    const endpoint = parse('https://example.com/telemetry');
-    const client = new EndpointTelemetrySink({ endpoint, ioHost });
+    const client = new EndpointTelemetrySink({ endpoint: 'https://example.com/telemetry', ioHost });
 
     // Mock https.request to throw an error
     (https.request as jest.Mock).mockImplementation(() => {

packages/aws-cdk/test/cli/telemetry/file-sink.test.ts

@@ -123,6 +123,14 @@ describe('FileTelemetrySink', () => {
     expect(fileContent).toBe(expectedSingleEvent + expectedSingleEvent);
   });
 
+  test('constructor throws if file already exists', async () => {
+    // GIVEN
+    fs.writeFileSync(logFilePath, 'exists');
+
+    // WHEN & THEN
+    expect(() => new FileTelemetrySink({ logFilePath, ioHost })).toThrow(/Telemetry file already exists/);
+  });
+
   test('handles errors gracefully and logs to trace without throwing', async () => {
     // GIVEN
     const testEvent: TelemetrySchema = {