feat(cwl): Add metric definitions for LiveTail #916
Conversation
| @@ -1297,6 +1297,11 @@ | |||
| "type": "boolean", | |||
| "description": "Whether the user has access to CodeWhisperer Chat" | |||
| }, | |||
| { | |||
| "name": "hasLogEventFilterPattern", | |||
There was a problem hiding this comment.
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.
yes I missed hasTextFilter which in itself is over specified and could have been filterType 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 hasLogEventFilterPattern
There was a problem hiding this comment.
Yeah we can use hasTextFilter.
| "name": "hasLogEventFilterPattern", | ||
| "type": "boolean", | ||
| "description": "If LogEvent filter pattern is applied" | ||
| }, | ||
| { | ||
| "name": "hasTextFilter", |
There was a problem hiding this comment.
How is hasLogEventFilterPattern different than this existing field?
| @@ -4995,6 +5010,48 @@ | |||
| } | |||
| ] | |||
| }, | |||
| { | |||
| "name": "cwlLiveTail_Start", | |||
There was a problem hiding this comment.
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.
my bad. I perceived this as a service on it's own. could we rename it to cloudwatchlogs_Start.
if needed later we could add field distinguishing as type:liveTail
There was a problem hiding this comment.
could we rename it to
cloudwatchlogs_Start.
I think cloudwatchlogs_startLiveTail would be fine. As long as the same prefix (cloudwatchlogs_) is used as other CWL metrics.
| { | ||
| "name": "logStreamFilterType", | ||
| "type": "string", | ||
| "description": "Type of LogStream filter applied to session" |
There was a problem hiding this comment.
@keeganirby can you also rename this to filterType, make it enum. also how is this information different than hasLogEventFilterPattern:true
There was a problem hiding this comment.
We have two different filter parameters:
- logStream: determines from which logStream(s) log events are included
- LogEvent: only includes logEvents that match a specified pattern.
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.
Conversely, there is no "type" to a logEventFilter. It is always a text pattern, thus why it is set as a boolean
There was a problem hiding this comment.
If we change the type's name to just filterType, and we have multiple filterType for a given request, how would that work?
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 filterType for LogEvents, how could we set both for the same request?
Also if I make this an enum, can I make the accepted values specific to CWL (logStream_prefix) or should it be generic (prefix)?
There was a problem hiding this comment.
But hypothetically if we were also emitting a filterType for LogEvents, how could we set both for the same request?
Would it be on a single metric?
if I make this an enum, can I make the accepted values specific to CWL (
logStream_prefix) or should it be generic (prefix)?
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.
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 filterType with allowedValues: ['all', 'prefix', 'specific']
## 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`
Problem
Cloudwatch wants to monitor usage metrics of the LiveTail integration
Solution
When starting a session emit telemetry for:
When closing a session:
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.