Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/core/src/awsService/cloudWatchLogs/activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor

@justinmk3 justinmk3 Oct 21, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

import { Settings } from '../../shared/settings'
import { addLogEvents } from './commands/addLogEvents'
import { copyLogResource } from './commands/copyLogResource'
Expand All @@ -20,11 +20,15 @@ import { changeLogSearchParams } from './changeLogSearch'
import { CloudWatchLogsNode } from './explorer/cloudWatchLogsNode'
import { loadAndOpenInitialLogStreamFile, LogStreamCodeLensProvider } from './document/logStreamsCodeLensProvider'
import { tailLogGroup } from './commands/tailLogGroup'
import { LiveTailDocumentProvider } from './document/liveTailDocumentProvider'
import { LiveTailSessionRegistry } from './registry/liveTailSessionRegistry'

export async function activate(context: vscode.ExtensionContext, configuration: Settings): Promise<void> {
const registry = LogDataRegistry.instance
const liveTailRegistry = LiveTailSessionRegistry.instance

const documentProvider = new LogDataDocumentProvider(registry)
const liveTailDocumentProvider = new LiveTailDocumentProvider()

context.subscriptions.push(
vscode.languages.registerCodeLensProvider(
Expand All @@ -40,6 +44,10 @@ export async function activate(context: vscode.ExtensionContext, configuration:
vscode.workspace.registerTextDocumentContentProvider(CLOUDWATCH_LOGS_SCHEME, documentProvider)
)

context.subscriptions.push(
vscode.workspace.registerTextDocumentContentProvider(CLOUDWATCH_LOGS_LIVETAIL_SCHEME, liveTailDocumentProvider)
)

context.subscriptions.push(
vscode.workspace.onDidCloseTextDocument((doc) => {
if (doc.isClosed && doc.uri.scheme === CLOUDWATCH_LOGS_SCHEME) {
Expand Down Expand Up @@ -97,7 +105,7 @@ export async function activate(context: vscode.ExtensionContext, configuration:
node instanceof LogGroupNode
? { regionName: node.regionCode, groupName: node.logGroup.logGroupName! }
: undefined
await tailLogGroup(logGroupInfo)
await tailLogGroup(liveTailRegistry, logGroupInfo)
})
)
}
133 changes: 129 additions & 4 deletions packages/core/src/awsService/cloudWatchLogs/commands/tailLogGroup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,142 @@
* SPDX-License-Identifier: Apache-2.0
*/

import * as vscode from 'vscode'
import { TailLogGroupWizard } from '../wizard/tailLogGroupWizard'
import { getLogger } from '../../../shared'
import { CancellationError } from '../../../shared/utilities/timeoutUtils'
import { LiveTailSession, LiveTailSessionConfiguration } from '../registry/liveTailSession'
import { LiveTailSessionRegistry } from '../registry/liveTailSessionRegistry'
import { LiveTailSessionLogEvent, StartLiveTailResponseStream } from '@aws-sdk/client-cloudwatch-logs'
import { ToolkitError } from '../../../shared'

export async function tailLogGroup(logData?: { regionName: string; groupName: string }): Promise<void> {
export async function tailLogGroup(
registry: LiveTailSessionRegistry,
logData?: { regionName: string; groupName: string }
): Promise<void> {
const wizard = new TailLogGroupWizard(logData)
const wizardResponse = await wizard.run()
if (!wizardResponse) {
throw new CancellationError('user')
}

//TODO: Remove Log. For testing while we aren't yet consuming the wizardResponse.
getLogger().info(JSON.stringify(wizardResponse))
const liveTailSessionConfig: LiveTailSessionConfiguration = {
logGroupName: wizardResponse.regionLogGroupSubmenuResponse.data,
logStreamFilter: wizardResponse.logStreamFilter,
logEventFilterPattern: wizardResponse.filterPattern,
region: wizardResponse.regionLogGroupSubmenuResponse.region,
}
const session = new LiveTailSession(liveTailSessionConfig)
if (registry.has(session.uri)) {
await prepareDocument(session)
return
}
registry.set(session.uri, session)

const document = await prepareDocument(session)
registerTabChangeCallback(session, registry, document)
const stream = await session.startLiveTailSession()

await handleSessionStream(stream, document, session)
}

export function closeSession(sessionUri: vscode.Uri, registry: LiveTailSessionRegistry) {
const session = registry.get(sessionUri)
if (session === undefined) {
throw new ToolkitError(`No LiveTail session found for URI: ${sessionUri.toString()}`)
}
session.stopLiveTailSession()
registry.delete(sessionUri)
}

export async function clearDocument(textDocument: vscode.TextDocument) {
const edit = new vscode.WorkspaceEdit()
const startPosition = new vscode.Position(0, 0)
const endPosition = new vscode.Position(textDocument.lineCount, 0)
edit.delete(textDocument.uri, new vscode.Range(startPosition, endPosition))
await vscode.workspace.applyEdit(edit)
}

async function prepareDocument(session: LiveTailSession): Promise<vscode.TextDocument> {
const textDocument = await vscode.workspace.openTextDocument(session.uri)
await clearDocument(textDocument)
await vscode.window.showTextDocument(textDocument, { preview: false })
await vscode.languages.setTextDocumentLanguage(textDocument, 'log')
return textDocument
}

async function handleSessionStream(
stream: AsyncIterable<StartLiveTailResponseStream>,
document: vscode.TextDocument,
session: LiveTailSession
) {
try {
for await (const event of stream) {
if (event.sessionUpdate !== undefined) {
const formattedLogEvents = event.sessionUpdate.sessionResults!.map<string>((logEvent) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

formatLogEvent(logEvent)
)
if (formattedLogEvents.length !== 0) {
await updateTextDocumentWithNewLogEvents(formattedLogEvents, document, session.maxLines)
}
}
}
} catch (err) {
throw new ToolkitError('Caught on-stream exception')
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack, will adress.

}
}

function formatLogEvent(logEvent: LiveTailSessionLogEvent): string {
if (!logEvent.timestamp || !logEvent.message) {
return ''
}
const timestamp = new Date(logEvent.timestamp).toLocaleTimeString('en', {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, will address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

timeStyle: 'medium',
hour12: false,
timeZone: 'UTC',
})
let line = timestamp.concat('\t', logEvent.message)
if (!line.endsWith('\n')) {
line = line.concat('\n')
}
return line
}

async function updateTextDocumentWithNewLogEvents(
formattedLogEvents: string[],
document: vscode.TextDocument,
maxLines: number
) {
const edit = new vscode.WorkspaceEdit()
formattedLogEvents.forEach((formattedLogEvent) =>
edit.insert(document.uri, new vscode.Position(document.lineCount, 0), formattedLogEvent)
)
await vscode.workspace.applyEdit(edit)
}

function registerTabChangeCallback(
session: LiveTailSession,
registry: LiveTailSessionRegistry,
document: vscode.TextDocument
) {
vscode.window.tabGroups.onDidChangeTabs((tabEvent) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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()) {

    }
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const isOpen = isLiveTailSessionOpenInAnyTab(session)
if (!isOpen) {
closeSession(session.uri, registry)
void clearDocument(document)
}
})
}

function isLiveTailSessionOpenInAnyTab(liveTailSession: LiveTailSession) {
let isOpen = false
vscode.window.tabGroups.all.forEach(async (tabGroup) => {
tabGroup.tabs.forEach((tab) => {
if (tab.input instanceof vscode.TabInputText) {
if (liveTailSession.uri.toString() === tab.input.uri.toString()) {
isOpen = true
}
}
})
})
return isOpen
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/*!
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

import * as vscode from 'vscode'

export class LiveTailDocumentProvider implements vscode.TextDocumentContentProvider {
provideTextDocumentContent(uri: vscode.Uri, token: vscode.CancellationToken): vscode.ProviderResult<string> {
//Content will be written to the document via handling a LiveTail response stream in the TailLogGroup command.
return ''
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
* SPDX-License-Identifier: Apache-2.0
*/
import * as vscode from 'vscode'
import { CloudWatchLogsClient, StartLiveTailCommand, StartLiveTailCommandOutput } from '@aws-sdk/client-cloudwatch-logs'
import {
CloudWatchLogsClient,
StartLiveTailCommand,
StartLiveTailResponseStream,
} from '@aws-sdk/client-cloudwatch-logs'
import { LogStreamFilterResponse } from '../wizard/liveTailLogStreamSubmenu'
import { CloudWatchLogsSettings } from '../cloudWatchLogsUtils'
import { Settings, ToolkitError } from '../../../shared'
Expand Down Expand Up @@ -58,12 +62,16 @@ export class LiveTailSession {
return this._logGroupName
}

public startLiveTailSession(): Promise<StartLiveTailCommandOutput> {
public async startLiveTailSession(): Promise<AsyncIterable<StartLiveTailResponseStream>> {
const command = this.buildStartLiveTailCommand()
try {
return this.liveTailClient.cwlClient.send(command, {
const commandOutput = await this.liveTailClient.cwlClient.send(command, {
abortSignal: this.liveTailClient.abortController.signal,
})
if (!commandOutput.responseStream) {
throw new ToolkitError('LiveTail session response stream is undefined.')
}
return commandOutput.responseStream
} catch (e) {
throw new ToolkitError('Encountered error while trying to start LiveTail session.')
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
/*!
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

import * as sinon from 'sinon'
import * as vscode from 'vscode'

import assert from 'assert'
import { clearDocument, closeSession, tailLogGroup } from '../../../../awsService/cloudWatchLogs/commands/tailLogGroup'
import { StartLiveTailResponseStream } from '@aws-sdk/client-cloudwatch-logs'
import { LiveTailSessionRegistry } from '../../../../awsService/cloudWatchLogs/registry/liveTailSessionRegistry'
import { LiveTailSession } from '../../../../awsService/cloudWatchLogs/registry/liveTailSession'
import { asyncGenerator } from '../../../../shared/utilities/collectionUtils'
import {
TailLogGroupWizard,
TailLogGroupWizardResponse,
} from '../../../../awsService/cloudWatchLogs/wizard/tailLogGroupWizard'
import { getTestWindow } from '../../../shared/vscode/window'

describe('TailLogGroup', function () {
const testLogGroup = 'test-log-group'
const testRegion = 'test-region'
const testMessage = 'test-message'

let sandbox: sinon.SinonSandbox
let registry: LiveTailSessionRegistry
let startLiveTailSessionSpy: sinon.SinonSpy
let stopLiveTailSessionSpy: sinon.SinonSpy
let wizardSpy: sinon.SinonSpy

beforeEach(function () {
sandbox = sinon.createSandbox()
registry = new LiveTailSessionRegistry()
})

afterEach(function () {
sandbox.restore()
})

it('starts LiveTailSession and writes to document. Closes tab and asserts session gets closed.', async function () {
wizardSpy = sandbox.stub(TailLogGroupWizard.prototype, 'run').callsFake(async function () {
return getTestWizardResponse()
})
Comment on lines +42 to +44
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

startLiveTailSessionSpy = sandbox
.stub(LiveTailSession.prototype, 'startLiveTailSession')
.callsFake(async function () {
return getTestResponseStream()
})
stopLiveTailSessionSpy = sandbox
.stub(LiveTailSession.prototype, 'stopLiveTailSession')
.callsFake(async function () {
return
})
await tailLogGroup(registry, {
groupName: testLogGroup,
regionName: testRegion,
})
assert.strictEqual(wizardSpy.calledOnce, true)
assert.strictEqual(startLiveTailSessionSpy.calledOnce, true)
assert.strictEqual(registry.size, 1)

//registry is asserted to have only one entry, so this is assumed to be the session that was
//started in this test.
let sessionUri: vscode.Uri | undefined
registry.forEach((session) => (sessionUri = session.uri))
if (sessionUri === undefined) {
throw Error
}
const document = getTestWindow().activeTextEditor?.document
assert.strictEqual(sessionUri.toString(), document?.uri.toString())
assert.strictEqual(document?.getText().trim(), `12:00:00\t${testMessage}`)

//Test that closing all tabs the session's document is open in will cause the session to close
const window = getTestWindow()
let tabs: vscode.Tab[] = []
window.tabGroups.all.forEach((tabGroup) => {
tabs = tabs.concat(getLiveTailSessionTabsFromTabGroup(tabGroup, sessionUri!))
})
await Promise.all(tabs.map((tab) => window.tabGroups.close(tab)))
assert.strictEqual(registry.size, 0)
assert.strictEqual(stopLiveTailSessionSpy.calledOnce, true)
})

it('closeSession removes session from registry and calls underlying stopLiveTailSession function.', function () {
stopLiveTailSessionSpy = sandbox
.stub(LiveTailSession.prototype, 'stopLiveTailSession')
.callsFake(async function () {
return
})
const session = new LiveTailSession({
logGroupName: testLogGroup,
region: testRegion,
})
registry.set(session.uri, session)

closeSession(session.uri, registry)
assert.strictEqual(0, registry.size)
assert.strictEqual(true, stopLiveTailSessionSpy.calledOnce)
})

it('clearDocument clears all text from document', async function () {
const session = new LiveTailSession({
logGroupName: testLogGroup,
region: testRegion,
})
const testData = 'blah blah blah'
const document = await vscode.workspace.openTextDocument(session.uri)
const edit = new vscode.WorkspaceEdit()
edit.insert(document.uri, new vscode.Position(0, 0), testData)
await vscode.workspace.applyEdit(edit)
assert.strictEqual(document.getText(), testData)

await clearDocument(document)
assert.strictEqual(document.getText(), '')
})

function getLiveTailSessionTabsFromTabGroup(tabGroup: vscode.TabGroup, sessionUri: vscode.Uri): vscode.Tab[] {
return tabGroup.tabs.filter((tab) => {
if (tab.input instanceof vscode.TabInputText) {
return sessionUri!.toString() === tab.input.uri.toString()
}
})
}

function getTestWizardResponse(): TailLogGroupWizardResponse {
return {
regionLogGroupSubmenuResponse: {
region: testRegion,
data: testLogGroup,
},
filterPattern: '',
logStreamFilter: {
type: 'all',
},
}
}

function getTestResponseStream(): AsyncIterable<StartLiveTailResponseStream> {
const sessionStartFrame: StartLiveTailResponseStream = {
sessionStart: {
logGroupIdentifiers: [testLogGroup],
},
sessionUpdate: undefined,
}
const sessionUpdateFrame: StartLiveTailResponseStream = {
sessionUpdate: {
sessionResults: [
{
message: testMessage,
timestamp: 876830400000,
},
],
},
}
return asyncGenerator([sessionStartFrame, sessionUpdateFrame])
}
})
Loading