-
Notifications
You must be signed in to change notification settings - Fork 52
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 metric definitions for LiveTail #916
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1297,6 +1297,11 @@ | |
"type": "boolean", | ||
"description": "Whether the user has access to CodeWhisperer Chat" | ||
}, | ||
{ | ||
"name": "hasLogEventFilterPattern", | ||
"type": "boolean", | ||
"description": "If LogEvent filter pattern is applied" | ||
}, | ||
{ | ||
"name": "hasTextFilter", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is |
||
"type": "boolean", | ||
|
@@ -1438,6 +1443,11 @@ | |
"type": "string", | ||
"description": "User locale. Examples: en-US, en-GB, etc." | ||
}, | ||
{ | ||
"name": "logStreamFilterType", | ||
"type": "string", | ||
"description": "Type of LogStream filter applied to session" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @keeganirby can you also rename this to filterType, make it enum. also how is this information different than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have two different filter parameters:
For LogStream filter, we have multiple "types" of filters. Being one of: 'prefix', 'all', or 'specific'. We want to capture what type of filter is being used in VSCode. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we change the type's name to just In our case this isn't much of an issue since the only filterType we have is for LogStreams. But hypothetically if we were also emitting a Also if I make this an enum, can I make the accepted values specific to CWL ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would it be on a single metric?
Feel free to add any and all useful values to an enum, but starting with generic 'prefix', 'all', 'specific' seems fine. Fields exist on metrics, thus they are inherently "namespaced" by whatever metric they are attached to. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah in a single metric. Again this is mainly a hypothetical, but we are currently emitting a single metric per "startLiveTail" request. If we wanted to emit a filterType for two different parameters on that request, that might be problematic. This isn't a blocker to this PR, so i'm moving forward with renaming to |
||
}, | ||
{ | ||
"name": "metricId", | ||
"type": "string", | ||
|
@@ -1647,6 +1657,11 @@ | |
"type": "string", | ||
"description": "The name of the AWS service acted on. These values come from the AWS SDK. To find them in the JAVA SDK search for SERVICE_NAME in each service client, or look for serviceId in metadata in the service2.json" | ||
}, | ||
{ | ||
"name": "sessionAlreadyStarted", | ||
"type": "boolean", | ||
"description": "Session already open that matches new request" | ||
}, | ||
{ | ||
"name": "sessionDuration", | ||
"type": "int", | ||
|
@@ -4995,6 +5010,42 @@ | |
} | ||
] | ||
}, | ||
{ | ||
"name": "cwlLiveTail_Start", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am certain that we have existing cloudwatch metrics. Yet these new "live tail" metrics don't use the same prefix, so now both will be harder to discover. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my bad. I perceived this as a service on it's own. could we rename it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think |
||
"description": "When user starts a new LiveTail command", | ||
"metadata": [ | ||
{ | ||
"type": "hasLogEventFilterPattern", | ||
"required": false | ||
}, | ||
{ | ||
"type": "logStreamFilterType", | ||
"required": false | ||
}, | ||
{ | ||
"type": "sessionAlreadyStarted", | ||
"required": true | ||
}, | ||
{ | ||
"type": "source", | ||
"required": true | ||
keeganirby marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
] | ||
}, | ||
{ | ||
"name": "cwlLiveTail_Stop", | ||
"description": "When user stops a liveTailSession", | ||
"metadata": [ | ||
{ | ||
"type": "duration", | ||
"required": true | ||
}, | ||
{ | ||
"type": "source", | ||
"required": true | ||
} | ||
] | ||
keeganirby marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
{ | ||
"name": "deeplink_open", | ||
"description": "User requested that a resource be opened in the browser using the deeplink service", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is over-specific. Can you find a field name that will be useful for other metrics in the future, e.g.
hasPatternFilter
(why is "log event" important enough to call out in the field name?)Another way of future-proofing is to use enum-like (or string) fields instead of booleans. It's pretty cumbersome that we have booleans for each of these variants, and may need to add even more in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I missed
hasTextFilter
which in itself is over specified and could have beenfilterType
with enum of text, time, log event etc. but at times metrics requires multiple fields each with it's corresponding boolean values. such has CW logs could have both time & text filter and hence we would go with two fields.In this case, I agree we could reuse hasTextFilter. @keeganirby could you please reuse
hasTextFilter
in codebase and then we can safely remove hasLogEventFilterPatternThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can use hasTextFilter.