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

adding scopes to auth metrics for detailed authStatus #736

Merged
merged 4 commits into from
May 17, 2024
Merged

adding scopes to auth metrics for detailed authStatus #736

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e099bd0
adding scopes to auth metrics for detailed authStatus
May 16, 2024
073cfd0
npm run fix
May 17, 2024
86e2f48
added authScopes to addConnection metric
May 17, 2024
40196b3
reworded auth_userState
May 17, 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
49 changes: 48 additions & 1 deletion telemetry/definitions/commonDefinitions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,26 @@
"type": "int",
"description": "Current number of authentication connections the user has"
},
{
"name": "authEnabledConnections",
"type": "string",
"description": "Comma-delimited list of enabled auths."
},
{
"name": "authScopes",
"type": "string",
"description": "Comma-delimited list of scopes that users has"
},
{
"name": "authStatus",
"type": "string",
"allowedValues": [
"connected",
"notConnected",
"expired"
],
"description": "Status of the an auth connection."
},
{
"name": "awsFiletype",
"type": "string",
Expand Down Expand Up @@ -1780,6 +1800,10 @@
"type": "attempts",
"required": false
},
{
"type": "authScopes",
"required": false
},
{
"type": "credentialSourceId"
},
Expand Down Expand Up @@ -1848,6 +1872,29 @@
}
]
},
{
"name": "auth_userState",
"description": "The state of the user's authentication.",
Copy link
Contributor

@justinmk3 justinmk3 May 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

metrics are for events. state isn't an event

What event does auth_userState represent? It looks more like data. Can the data be stored on an existing event? Otherwise, it's not clear when this "event" should be emitted, it would be more like a heartbeat (which then implies it should be on... an existing, semi-frequent event)

If this is to be done on startup, can it be added to the existing startup event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's only on startup.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this is upstreaming an existing metric: https://github.com/aws/aws-toolkit-vscode/blob/0f9782a9d8407d3d755c44dc3a3eec1fe405e7b9/packages/core/src/shared/telemetry/vscodeTelemetry.json#L1108-L1109

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes Ideally it should be in common only because JetBrains is also emitting this metric. Trying to fix it by moving it to common.

"metadata": [
{
"type": "authEnabledConnections",
"required": true
},
{
"type": "authScopes",
"required": false
},
{
"type": "authStatus",
"required": true
},
{
"type": "source",
"required": true
}
],
"passive": true
},
{
"name": "aws_copyArn",
"description": "Copy the ARN of an AWS resource",
Expand Down Expand Up @@ -5753,4 +5800,4 @@
]
}
]
}
}
Loading