Skip to content
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

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

Merged
merged 5 commits into from
Nov 8, 2024

Conversation

keeganirby
Copy link
Contributor

Problem

Users may want to clear their screen during a tailing session. The liveTail document is read only.
Users may want to stop their session without closing the document.

Solution

Provide a codeLens to clear the screen (make document empty)
Provide a codeLens to stop the session without closing the document.

Moves Interval Timer for updating the StatusBar to the LiveTailSession object so stopLiveTailSession() can interrupt it, and be guaranteed to be cleaned up.

The TextDocument's URI and Session URI seem to not be equal. Looking up in the LiveTailSessionRegistry with a document URI causes a session to not be found. Converting with toString allows these URIs to match and the registry to work as intended.

Improves Exception handling on-stream. Previously, stopping the session in an expected fashion (codeLens, Closing editors) would cause an exception to bubble up and appear to the User. New change recognizes when the Abort Controller triggers, signifying an error has not occured, and logs the event as opposed to surfacing an error. Exposing only isAborted so that a caller has to use stopLiveTailSession to trigger the abortController and guarantee that other clean up actions have taken place.


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

@keeganirby keeganirby requested a review from a team as a code owner November 8, 2024 03:51
Copy link

github-actions bot commented Nov 8, 2024

  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

@@ -32,32 +32,29 @@ export async function tailLogGroup(
region: wizardResponse.regionLogGroupSubmenuResponse.region,
}
const session = new LiveTailSession(liveTailSessionConfig)
if (registry.has(session.uri)) {
if (registry.has(session.uri.toString())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

uriToKey exists for this purpose.

export function uriToKey(uri: vscode.Uri): string {

toString is not reliable, querystring parameters can appear in arbitrary order.

Copy link
Contributor

Choose a reason for hiding this comment

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

public isRegistered(uri: vscode.Uri): boolean {
return this.registry.has(uriToKey(uri))
}

This makes me wonder why LiveTailSessionRegistry is using a completely different pattern than LogDataRegistry. Now we have two different "registries" concepts to maintain, using completely different patterns.

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 think the aim of these two registries are different from one another.
Given a LiveTail session has its own components unique to other CWL experiences (own client, abortController, responseStream, etc.) We have a LiveTailSession object, that has a long running lifeCycle. This lifecycle can end from outside the TailLogGroup command, and so this registry is really just there to track what Sessions are currently running, so when a session needs to be closed via a CodeLens, or closing its textEditors, we can find the LiveTailSession from its uri and abort the response stream and clean up its resources.

I think this is a separate responsiblity from the LogDataRegistry that is actually persisting LogData, and making requests to FilterLogEvents.

Comment on lines +56 to +58
scheme: cloudwatchLogsLiveTailScheme,
},
new LiveTailCodeLensProvider()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have a different scheme, and a different codelens, for "live tail"? The codelenses for the non-livetail log group search would be useful in live tail. Why are they separate?

Whether a log group is being "tailed" or not is just a flag. It's not an entirely different concept.

Copy link
Contributor Author

@keeganirby keeganirby Nov 8, 2024

Choose a reason for hiding this comment

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

The CodeLenses in the existing CWL experiences are not very applicable to our LiveTail use case.
"Load newer" & "Load older" don't apply when data is being streamed in constantly. For View full log stream, customers can apply LogStream filter parameters to their liveTail session already so having this as a codeLens in a Livetail session doesn't seem necessary.
On the other hand, a SearchLogGroup command won't need a Stop tailing code lens.

These UX are quite separate.

We already separate the CodeLens providers for logData and logStream. Given the UX and functionality of LiveTail is different from our other experiences I think it fits to have a separate CodeLens provider.

This is the same reasoning behind having a separate scheme as we discussed here. The way we retrieve/display logs from a LiveTail session stream differs enough from the existing LogDataRegistry that I felt that it was best to separate these logic's, as opposed to finding a way to generalize.

I feel that LiveTail and the existing experiences are separate enough from each other that they should be treated as such.

@justinmk3 justinmk3 merged commit e6645d4 into aws:feature/cwltail Nov 8, 2024
23 of 25 checks passed
karanA-aws pushed a commit to karanA-aws/aws-toolkit-vscode that referenced this pull request Jan 17, 2025
Users may want to clear their screen during a tailing session. The
liveTail document is read only.
Users may want to stop their session without closing the document.

Provide a codeLens to clear the screen (make document empty)
Provide a codeLens to stop the session without closing the document.

Moves Interval Timer for updating the StatusBar to the LiveTailSession
object so `stopLiveTailSession()` can interrupt it, and be guaranteed to
be cleaned up.

The TextDocument's URI and Session URI seem to not be equal. Looking up
in the LiveTailSessionRegistry with a document URI causes a session to
not be found. Converting with `toString` allows these URIs to match and
the registry to work as intended.

Improves Exception handling on-stream. Previously, stopping the session
in an expected fashion (codeLens, Closing editors) would cause an
exception to bubble up and appear to the User. New change recognizes
when the Abort Controller triggers, signifying an error has not occured,
and logs the event as opposed to surfacing an error. Exposing only
`isAborted` so that a caller has to use `stopLiveTailSession` to trigger
the abortController and guarantee that other clean up actions have taken
place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants