feat(cwl): initialize tailLogGroup command. starts stream and prints logEvents to textDocument #5790

keeganirby · 2024-10-15T23:33:18Z

Problem

TailLogGroup currently is just logging the Users WizardResponse. It does not start a LiveTail stream or print results to the VSCode editor

Solution

Creates a LiveTailSession with the input params from the TailLogGroupWizard
Registers the LiveTailSession in the LiveTailSessionRegistry
Iterates the Response Stream and prints new LogEvents to a TextDocument
If there are no open tabs for a given session's TextDocument, the stream will be closed by triggering its AbortController and disposing its client.

We will use a new URI Scheme for LiveTail text documents so we can identify a LiveTail session vs a Cloudwatch SearchLogGroup/ViewLogStream document. In the future, we will register LiveTail specific CodeLenses that depend upon this URI scheme. Defining a new URI scheme for a Text Document requires a Document Provider. In this use case, it is empty given that the response handling that writes to the document lives within the TailLogGroup command itself.

License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

…o a TextDocument

github-actions · 2024-10-15T23:33:32Z

This pull request modifies code in src/ but no tests were added/updated. Confirm whether tests should be added or ensure the PR description explains why tests are not required.

github-actions · 2024-10-15T23:33:32Z

This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions.

keeganirby · 2024-10-15T23:56:45Z

Spoke with Josh about this during the initial PR: will add a Changelog when the feature branch is complete and ready to merge to master.

jpinkney-aws · 2024-10-18T19:33:26Z

packages/core/src/awsService/cloudWatchLogs/commands/tailLogGroup.ts

+    try {
+        for await (const event of stream) {
+            if (event.sessionUpdate !== undefined) {
+                const formattedLogEvents = event.sessionUpdate.sessionResults!.map<string>((logEvent) =>


It looks like you're using ! in sessionResults! to assert that sessionResults is never undefined. Is that actually the case? Technically if sessionResults returns undefined one time then it results in an error

You're right! I think I had this for some early testing and never when back. I'll add event.sessionUpdate.sessionResults !== undefined to the if check.

jpinkney-aws · 2024-10-18T19:37:14Z

packages/core/src/awsService/cloudWatchLogs/commands/tailLogGroup.ts

+    registry: LiveTailSessionRegistry,
+    document: vscode.TextDocument
+) {
+    vscode.window.tabGroups.onDidChangeTabs((tabEvent) => {


If i'm understanding this correctly I think you might want vscode.workspace.textDocuments instead. I've never actually seen vscode.window.tabGroups before 🤔. The process would be more or less the same expect instead of onDidChangeTabs and interating over every tab group you can just. Something like:

vscode.workspace.onDidChangeTextDocument((textDocument) => { if (textDocument.document.uri.toString() === liveTailSession.uri.toString()) { } })

What we are trying to achieve is that if a user doesn't have the LiveTail session open in any tab in their editor, that we close the session.

What I've learned is that the underlying TextDocument can remain open, even if its editor tabs are closed. It seems that the LiveTail command will keep the doucment open even if all its editors are closed because the command is still writing from the stream to the document. https://code.visualstudio.com/api/references/vscode-api#workspace.onDidCloseTextDocument. This suggests looking at onDidChangeVisibleTextEditors, but that event fires even when swapping tabs, not closing. https://code.visualstudio.com/api/references/vscode-api#window.onDidChangeVisibleTextEditors

This implementation is admittedly heavy, but has been the only thing I've found that fits what I'm trying to accomplish.

justinmk3 · 2024-10-21T17:49:06Z

packages/core/src/awsService/cloudWatchLogs/activation.ts

@@ -4,7 +4,7 @@
 */

 import * as vscode from 'vscode'
-import { CLOUDWATCH_LOGS_SCHEME } from '../../shared/constants'
+import { CLOUDWATCH_LOGS_LIVETAIL_SCHEME, CLOUDWATCH_LOGS_SCHEME } from '../../shared/constants'


please rename CLOUDWATCH_LOGS_LIVETAIL_SCHEME to conform to the lint rule. CLOUDWATCH_LOGS_SCHEME was marked as "ignore lint rule" as a temporary workaround, not something to continue when new symbols are introduced.

Why is a new scheme needed? Why not use a querystring parameter with the existing aws-cwl:// scheme?

I will rename, didn't fully realize the linter suppression, was just following precedent. I can update the variable naming.

The reason I added a new scheme is because I am not leveraging the existing document and code lens providers. The logic for fetching logs, and the supplied codeLenses for LiveTail are different than those in the existing providers. I didn't know query param would be an option, but since the providers are tied to the scheme itself, I'm not sure if a query param does what I was intending for by having separate providers. Let me know if that can be done!

I could use a query param and a shared scheme, but then I assume that each time one of the providers run, we'd need to inspect the URI and have a branching logic to handle a LiveTail case or not. In that case I'd prefer to just handle these cases in separate classes.

I am not leveraging the existing document and code lens providers

I believe the existing abstraction is generalized to allow different "providers" of CWL events. So it could be re-used. But it may be a judgement call about whether that saves or increases complexity.

justinmk3 · 2024-10-21T17:52:02Z

packages/core/src/awsService/cloudWatchLogs/commands/tailLogGroup.ts

+            }
+        }
+    } catch (err) {
+        throw new ToolkitError('Caught on-stream exception')


what is the point of this? the exception that was caught likely has more useful information.

please review all of the code guidelines: https://github.com/aws/aws-toolkit-vscode/blob/master/docs/CODE_GUIDELINES.md#exceptions

ack, will adress.

justinmk3 · 2024-10-21T17:53:53Z

packages/core/src/awsService/cloudWatchLogs/commands/tailLogGroup.ts

+    if (!logEvent.timestamp || !logEvent.message) {
+        return ''
+    }
+    const timestamp = new Date(logEvent.timestamp).toLocaleTimeString('en', {


use one of the existing shared datetime format utils. that ensures that we use consistent formatting everywhere in the extension:

aws-toolkit-vscode/packages/core/src/shared/utilities/textUtilities.ts

Lines 284 to 303 in 6fa3a9e

export function formatLocalized(d: Date = new Date(), cloud9 = isCloud9()): string {

const dateFormat = new Intl.DateTimeFormat('en-US', {

year: 'numeric',

month: 'short',

day: 'numeric',

})

const timeFormat = new Intl.DateTimeFormat('en-US', {

hour: 'numeric',

minute: 'numeric',

second: 'numeric',

timeZoneName: cloud9 ? 'short' : 'shortOffset',

})

return `${dateFormat.format(d)} ${timeFormat.format(d)}`

}

/**

* Matches Insights console timestamp, e.g.: 2019-03-04T11:40:08.781-08:00

* TODO: Do we want this this verbose? Log stream just shows HH:mm:ss

*/

export function formatDateTimestamp(forceUTC: boolean, d: Date = new Date()): string {

Ack, will address.

Circling back to this, when I demoed LiveTail to our CWL product manager, it was requested that we shorten the time we display to just HH:MM:SS. The reason being that these events are coming in live, so the additional context of Date and timezone weren't seen as important.

In that case, is this a required change to make?

justinmk3 · 2024-10-21T18:00:18Z

packages/core/src/test/awsService/cloudWatchLogs/commands/tailLogGroup.test.ts

+        wizardSpy = sandbox.stub(TailLogGroupWizard.prototype, 'run').callsFake(async function () {
+            return getTestWizardResponse()
+        })


we have a full wizard testing framework that allows the actual wizard code to run, and you can assert its state. please do not mock/spy things unless it's unavoidable (typically network/service calls).

I have the TailLogGroup wizard under test in its own Test class https://github.com/aws/aws-toolkit-vscode/blob/feature/cwltail/packages/core/src/test/awsService/cloudWatchLogs/wizard/tailLogGroupWizard.test.ts
which is why i decided to Mock it here. This tailLogGroup.test was intended to test just the functionality of the command itself.

In that case is it acceptable to leave this mocked? I can use that testing framework here also, was just trying to separate responsibilities of each testing class.

was just trying to separate responsibilities of each testing class.

Testing more things in one test can often be a benefit (less code, more coverage). Would it make sense to merge this test into the other?

Or meanwhile, people tend to copy what exists. So the presence of mocks here encourages even more mocks...

Sure, at the least I can copy over a basic form of the Wizard tester here to avoid mocking it. If I have a wizard tester in my test, how would you recommend supplying that to my Command? Doing local testing and the prompter still comes up when the test runs, waiting for input.

Is it recommended to use something like dependency injection to provide a TailLogGroupWizard to the command in its constructor so the test can supply its Wizard to the command?

I've added

const wizard = new TailLogGroupWizard({ groupName: testLogGroup, regionName: testRegion, }) const tester = await createWizardTester(wizard) tester.regionLogGroupSubmenuResponse.assertDoesNotShow() tester.logStreamFilter.assertShowFirst() tester.filterPattern.assertShowSecond()

to my test before executing the command await tailLogGroup(registry, { groupName: testLogGroup, regionName: testRegion, }), but its hanging waiting for input on the Wizard because the command is creating a new Wizard and calling prompt on it.

but its hanging waiting for input on the Wizard because the command is creating a new Wizard and calling prompt on it

the wizard tester has functions to choose items from it. look at examples in other tests

…logEvents to textDocument (aws#5790) TailLogGroup currently is just logging the Users WizardResponse. It does not start a LiveTail stream or print results to the VSCode editor * Creates a `LiveTailSession` with the input params from the TailLogGroupWizard * Registers the LiveTailSession in the `LiveTailSessionRegistry` * Iterates the Response Stream and prints new LogEvents to a TextDocument * If there are no open tabs for a given session's TextDocument, the stream will be closed by triggering its AbortController and disposing its client. We will use a new URI Scheme for LiveTail text documents so we can identify a LiveTail session vs a Cloudwatch SearchLogGroup/ViewLogStream document. In the future, we will register LiveTail specific CodeLenses that depend upon this URI scheme. Defining a new URI scheme for a Text Document requires a Document Provider. In this use case, it is empty given that the response handling that writes to the document lives within the TailLogGroup command itself.

initialize tailLogGroup command. starts stream and prints logEvents t…

aa2ef2a

…o a TextDocument

keeganirby requested a review from a team as a code owner October 15, 2024 23:33

add tailLogGroup tests

f5efbd3

keeganirby force-pushed the release branch from 2b53a92 to a0321ec Compare October 17, 2024 18:06

fix close tab race condition in tailLogGroup test

19ef62c

keeganirby force-pushed the release branch from a0321ec to 19ef62c Compare October 17, 2024 20:28

trim document text in unit test to make more consistent across platforms

29c4a07

jpinkney-aws reviewed Oct 18, 2024

View reviewed changes

Keegan Irby added 2 commits October 18, 2024 12:51

assert that sessionResults are not undefined

d44fb4c

Comment why we iterate tabGroups to close session

7a41066

jpinkney-aws approved these changes Oct 21, 2024

View reviewed changes

jpinkney-aws merged commit 405f456 into aws:feature/cwltail Oct 21, 2024
23 of 24 checks passed

justinmk3 reviewed Oct 21, 2024

View reviewed changes

keeganirby mentioned this pull request Nov 8, 2024

feat(cwl): Add clear screen and stop session code lens #5958

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cwl): initialize tailLogGroup command. starts stream and prints logEvents to textDocument #5790

feat(cwl): initialize tailLogGroup command. starts stream and prints logEvents to textDocument #5790

keeganirby commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

keeganirby commented Oct 15, 2024

jpinkney-aws Oct 18, 2024

keeganirby Oct 18, 2024

jpinkney-aws Oct 18, 2024

keeganirby Oct 18, 2024

justinmk3 Oct 21, 2024 •

edited

Loading

keeganirby Oct 21, 2024

justinmk3 Oct 21, 2024

justinmk3 Oct 21, 2024

keeganirby Oct 21, 2024

justinmk3 Oct 21, 2024

keeganirby Oct 21, 2024

keeganirby Oct 21, 2024

justinmk3 Oct 21, 2024

keeganirby Oct 21, 2024

justinmk3 Oct 21, 2024

keeganirby Oct 21, 2024

justinmk3 Oct 22, 2024

	export function formatLocalized(d: Date = new Date(), cloud9 = isCloud9()): string {
	const dateFormat = new Intl.DateTimeFormat('en-US', {
	year: 'numeric',
	month: 'short',
	day: 'numeric',
	})
	const timeFormat = new Intl.DateTimeFormat('en-US', {
	hour: 'numeric',
	minute: 'numeric',
	second: 'numeric',
	timeZoneName: cloud9 ? 'short' : 'shortOffset',
	})

	return `${dateFormat.format(d)} ${timeFormat.format(d)}`
	}
	/**
	* Matches Insights console timestamp, e.g.: 2019-03-04T11:40:08.781-08:00
	* TODO: Do we want this this verbose? Log stream just shows HH:mm:ss
	*/
	export function formatDateTimestamp(forceUTC: boolean, d: Date = new Date()): string {

feat(cwl): initialize tailLogGroup command. starts stream and prints logEvents to textDocument #5790

feat(cwl): initialize tailLogGroup command. starts stream and prints logEvents to textDocument #5790

Conversation

keeganirby commented Oct 15, 2024 • edited Loading

Problem

Solution

github-actions bot commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

keeganirby commented Oct 15, 2024

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

justinmk3 Oct 21, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keeganirby commented Oct 15, 2024 •

edited

Loading

justinmk3 Oct 21, 2024 •

edited

Loading