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

telemetry: Add windowId common definition type #832

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nkomonen-amazon
Copy link
Contributor

@nkomonen-amazon nkomonen-amazon commented Aug 26, 2024

Problem:

I have filtered all the metrics in Kibana by clientId: "XXXXXX", but if they had multiple IDE windows opened at the same time sending metrics I cannot know which window they are coming from since they share the same clientID.

Solution

This adds a new type called viewId which uniquely identifies
a part of the UI in an IDE window.

The viewId should be a comma delimited string where each item
appended is a more specific part of the UI within the previous
item.

The top level (first item) part of the string should be an identifier
for the specific IDE window, as it is possible to have multiple windows
of the same IDE opened at the same time. Then if there is a specific
view within that specific IDE window you would append its identifier.

Example:

  • I have 2 VS Code windows with IDs: abc-123 and xyz-789
  • My Amazon Q chat view has an ID amazonq-chat
  • Result viewId: abc-123,amazonq-chat and xyz-789,amazonq-chat

Now in my metrics I will be able to differentiate metrics by their viewId
since they both share the same clientId by design.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -1468,6 +1468,11 @@
"type": "string",
"description": "A generic version metadata"
},
{
"name": "windowId",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could source be used for this intent? I assume not, because sometimes a metric would have an associated window and an upstream reason for the event.

I think we should generalize this slightly, by renaming it to viewId. Some windows may host multiple views, and we should be able to attribute events to those views.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could source be used for this intent?

source is more of a "semantic" concept (it describes a UI element or feature name) whereas (I assume) windowId/viewId will be an integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points.

Could source be used for this intent?

The reason for why you assumed we cannot is correct.

I think we should generalize this slightly, by renaming it to viewId

Would it make sense to have both windowId and viewId?

windowId - the ID of the IDE window
viewId - the ID of the view within the windowId

Eg:

{
  "windowId": "abc-123",
  "viewId": "amazon-q-chat"
}

A viewId will be static I would think, but the windowId will be dynamic.


As for using source I think this would not be reuseable for either as in VSC we use it for other purposes such as "which function caused this metric to emit", it is a generalize field.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to me that we can't just use source

I could see defining both window and view as metadata. I think this is going to be confusing for developers though ("which field do I use and what do they mean?"), and if we can boil it down to a singular field, we're more likely to avoid errors caused by ambiguity. Since a window is a view, can we have a single field, but if a product wants to be specific, they can codify that in the field?

For example, if the list of cloudwatch log streams was being shown within both the Lambda Function Window and the CloudWatch Log Groups Window, you'd differentiate using a viewId like "lambda.cwlstreams" and "cwlgroups.cwlstreams".

Copy link
Contributor Author

@nkomonen-amazon nkomonen-amazon Aug 26, 2024

Choose a reason for hiding this comment

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

Yeah makes sense to me. So in my case would this match your expectation for it to look something like:

viewId: "38061227-ee4a-4be2-b869-5521fad9f0731724683347178.lambda.cwlstreams"
vs
viewId: "38061227-ee4a-4be2-b869-5521fad9f0731724683347178.cwlgroups.cwlstreams"

Where the UUID is the IDE window ID, but then we append the underlying "view" id.

Even if there is not a value for the "view" part (lambda.cwlstreams), there will always be the IDE window ID (38061227-ee4a-4be2-b869-5521fad9f0731724683347178) in every metric. This is the justification for it being "global", as all metrics will come from a specific IDE window.

Copy link
Contributor

Choose a reason for hiding this comment

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

This examples match how I imagine the field would be used.

I think all of the toolkits differ in implementation details, but not all systems are aware of the view they are performing operations for. How are you planning to obtain these values without requiring coupling between view and controller?

Copy link
Contributor Author

@nkomonen-amazon nkomonen-amazon Aug 26, 2024

Choose a reason for hiding this comment

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

As for VSC, we can currently wrap a function in a "context" with our span/run mechanisms. This allows a high level caller such as the view to set a viewId field, and then deep within the call stack when we eventually need to resolve the viewId it will return the value set by the high level call. This is a cleaner alternative to just passing viewId as an argument to every nested function call.

Eventually when we need to resolve the final viewId before sending the event to our server, we would build a string from the current IDE window UUID + viewId set by the high level caller


For now, we would just set the window UUID in the viewId field, and then later look to solve to problem of also attaching the "view" source to it as that will take more work.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be at compute level right? if I'm VSC local user and I run multiple ec2 from local env, we should be able to identify these ec2s. how about computeEnvId / computeId or instanceId?

Copy link
Contributor

Choose a reason for hiding this comment

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

why can't we use instanceId and elementId as separate fields, since elementId is designed to point a particular ui element. is it because you are adding this field to all metrics? (along with product, os, version?)

Copy link
Contributor Author

@nkomonen-amazon nkomonen-amazon Aug 27, 2024

Choose a reason for hiding this comment

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

The previous discussion regarding a view ID for a specific view within an IDE is not relevant to this PR anymore. See Chris' comments down below.

The plan is to add a sessionId adjacent to clientId which is a unique ID to a specific IDE window, and is attached to all metrics (currently optional until all IDEs can support it). So if there are other windows that are ssh'd to EC2 for example, they will have their own unique sessionId. On top of that they will have the ec2 computeEnv so we can know how many different EC2 specific IDE windows that are using.

{
"name": "windowId",
"type": "string",
"description": "An identifier for a specific IDE window, since there can be multiple windows from the same IDE open on a machine."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "An identifier for a specific IDE window, since there can be multiple windows from the same IDE open on a machine."
"description": "Identifies an IDE window. There can be multiple windows from the same IDE open on a machine. Example: ..."

@justinmk3 justinmk3 changed the title feat: Add windowId common definition type telemetry: Add windowId common definition type Aug 26, 2024
@justinmk3
Copy link
Contributor

justinmk3 commented Aug 26, 2024

The plan is to attach this value to all telemetry events.

Should it go in MetricBase then? Or on the metric itself (instead of "metadata")?

@nkomonen-amazon
Copy link
Contributor Author

@justinmk3 just looked in to how we do our clientId field.

There are separate definitions in service-model.json, where we define the type and also assign it to the PostMetricsRequest type. This is where we set OS, OSVersion, ComputeEnv, ClientId.
Now I'm thinking it may make more sense to define windowId in here since the plan is to attach it to all events.

This is separate from how we do things for MetricBase.

@justinmk3
Copy link
Contributor

justinmk3 commented Aug 26, 2024

For now, we would just set the window UUID in the viewId field

will a UUID be useful in telemetry? If the client maps UUIDs to integers starting from 1, that's probably more meaningful in telemetry, no? That will make it intuitive in dashboards to see "this was the main window", "these are other windows"...

@awschristou
Copy link
Contributor

For now, we would just set the window UUID in the viewId field

will a UUID be useful in telemetry? If the client maps UUIDs to integers starting from 1, that's probably more meaningful in telemetry, no? That will make it intuitive in dashboards to see "this was the main window", "these are other windows"...

Neither is meaningful in the context of a dashboard, if you plot int or UUID as an axis. You'd want some sort of compact but human readable name. OTOH if you're making a dashboard that filters to this value, the human readable name is less stable, since humans will change it over time.

@nkomonen-amazon
Copy link
Contributor Author

nkomonen-amazon commented Aug 26, 2024

The current scenario we have is:

  • I have 2 VS Code windows open on my machine
  • Each Window has Amazon Q
  • By design, they share the same Client ID
  • When either of them emits a metric, we do not know which VS Code window it came from

As a solution, if we have a viewId it will allow me to know which metric came from which window. Whether the value is a UUID or a simple int, the value doesn't matter. All I care to know that if I have multiple metric events from the same clientId, that I can differentiate that, eg aws_refreshCredentials, came from IDE window 1 vs IDE window 2.

client maps UUIDs to integers starting from 1

This would make things nicer to read, but requires state management which I would assume we want less of.

This adds a new type called `viewId` which uniquely identifies
a part of the UI in an IDE window.

The `viewId` should be a comma delimited string where each item
appended is a more specific part of the UI within the previous
item.

The top level (first item) part of the string should be an identifier
for the specific IDE window, as it is possible to have multiple windows
of the same IDE opened at the same time. Then if there is a specific
view within that specific IDE window you would append its identifier.

Example:

- I have 2 VS Code windows with IDs: `abc-123` and `xyz-789`
- My Amazon Q chat view has an ID `amazonq-chat`
- Result `viewId`: `abc-123,amazonq-chat` and `xyz-789,amazonq-chat`

Now in my metrics I will be able to differentiate metrics by their viewId
since they both share the same clientId by design.

Signed-off-by: Nikolas Komonen <nkomonen@amazon.com>
@justinmk3
Copy link
Contributor

justinmk3 commented Aug 26, 2024

client maps UUIDs to integers starting from 1

This would make things nicer to read, but requires state management which I would assume we want less of.

Shouldn't. Just sort all known windows and use their ordinals. If UUIDs get "swapped out" at runtime it doesn't really matter. The common case will be "1", so when we see "2" or more, that tells us something (which would be difficult to deduce otherwise).

And this would allow a split series chart to visualize this. Can't do that with UUIDs.

@awschristou
Copy link
Contributor

Oh, are you talking about differentiating window instances ?

image

I thought you were referring to different view panels within the extension...

image

If that is the case, this is something that has been discussed on the team a few years ago, but never got around to. We should make a top-level sessionId field, which would be a UUID, uniquely produced each time the IDE is launched. This allows us to associate all operations with one specific session. This would live at the same level as clientId (service.json).

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.

4 participants