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

fix(cwl): Rename LiveTail metric definitions #932

Merged
merged 2 commits into from
Dec 5, 2024
Merged

Conversation

keeganirby
Copy link
Contributor

Problem

Addresses feedback on #916.

Solution

  • Uses hasTextFilter instead of defining a new hasLogEventFilterPattern property
  • Defines a more generic filterPattern type, as an enum that supports logStream filter type strings
  • renames metrics to cloudwatchlogs_startLiveTail and cloudwatchlogs_stopLiveTail

License

By submitting this pull request, 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 December 5, 2024 18:04
"type": "result"
},
{
"type": "sessionAlreadyStarted",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI: if "sessionAlreadyStarted" is a kind of failure, it's strongly recommended to simply throw an exception and let telemetry.xx.run(...) automatically set result=Failed and reason=<code> where <code> is the error code. https://github.com/aws/aws-toolkit-vscode/blob/016e478399f4e4e050e5e644c98c96297930e8ad/packages/core/src/shared/errors.ts#L144

That has other benefits: all failures can be captured in dashboards/queries easily, without needing to know specific, special fields such as this one.

However, if this is only an informational field and doesn't represent a failure or cancellation, then the above doesn't apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't consider this to be a failure case. This would be an informational field. The telemetry.xx.run(...) will not raise an exception.

@justinmk3 justinmk3 merged commit b047c14 into aws:main Dec 5, 2024
6 of 7 checks passed
justinmk3 pushed a commit to aws/aws-toolkit-vscode that referenced this pull request Dec 7, 2024
## Problem
Updating telemetry to accommodate changes in
aws/aws-toolkit-common#932

## Solution
Update package version for telemetry. 
Add validation on the LogStreamFilter submenu's response for filter
type. This is needed to allow the type returned from the
LogStreamFilterSubmenu to be convertible to the type in the metric
definition for filterPattern. In any case, this is a good validation to
have since the 'menu' placeholder value isn't valid for starting a
LiveTail session anyways.
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.

2 participants