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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Nov 19, 2024
Merged

feat(cwl): Add metric definitions for LiveTail #916

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64b9124
Add metric definitions for LiveTail
Nov 19, 2024
6b185c3
Reorder metrics and types
Nov 19, 2024
3878ba9
move cwlLiveTail
Nov 19, 2024
a8d263e
add result to livetail metrics
Nov 19, 2024
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
57 changes: 57 additions & 0 deletions telemetry/definitions/commonDefinitions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1297,6 +1297,11 @@
"type": "boolean",
"description": "Whether the user has access to CodeWhisperer Chat"
},
{
"name": "hasLogEventFilterPattern",
Copy link
Contributor

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.

Copy link
Contributor

@dhruvigajjar dhruvigajjar Dec 2, 2024 •
edited
Loading

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

Copy link
Contributor Author

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.

"type": "boolean",
"description": "If LogEvent filter pattern is applied"
},
{
"name": "hasTextFilter",
Copy link
Contributor

Choose a reason for hiding this comment

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

How is hasLogEventFilterPattern different than this existing field?

"type": "boolean",
Expand Down Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The 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 hasLogEventFilterPattern:true

Copy link
Contributor Author

@keeganirby keeganirby Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 filterType with allowedValues: ['all', 'prefix', 'specific']

},
{
"name": "metricId",
"type": "string",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -4995,6 +5010,48 @@
}
]
},
{
"name": "cwlLiveTail_Start",
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 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 cloudwatchlogs_Start.
if needed later we could add field distinguishing as type:liveTail

Copy link
Contributor

@justinmk3 justinmk3 Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

"description": "When user starts a new LiveTail command",
"metadata": [
{
"type": "hasLogEventFilterPattern",
"required": false
},
{
"type": "logStreamFilterType",
"required": false
},
{
"type": "result"
},
{
"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": "result"
},
{
"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",
Expand Down