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

added ComputeEnv as type with dev, local, test, web, cc, cloud9, wsl,… #665

Merged
merged 4 commits into from
Jan 16, 2024

Conversation

dhruvigajjar
Copy link
Contributor

Problem

Problem : Identify the ENV in metrics. Since different env have different user flows, this field becomes helpful in understanding why some metrics are emitted or better understand user flow w.r.t to ENVS

initially adding "ec2", "codecatalyst", "cloud9", "wsl", "test", "vscode-web", "sagemaker", etc.
we can add more later
field name = computeEnv, analogous to the existing computeRegion field

Solution

For all Toolkits to utilize field, adding ComputeEnv Field in PostMetricsRequest & PostFeedbackRequest

License

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

@dhruvigajjar dhruvigajjar requested a review from a team as a code owner January 8, 2024 18:05
Comment on lines 70 to 71
"cloud9",
"codeCatalyst",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need a type for a Cloud9 + CodeCatalyst?

Copy link
Contributor

Choose a reason for hiding this comment

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

since this is (currently) an enum instead of a list, yes. there should be a cloud9Codecatalyst item

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what are other combinations? if env can be combined & there are multiple combinations, it is good idea to instrument on Tookits' code insead?

Copy link
Contributor

@justinmk3 justinmk3 Jan 10, 2024

Choose a reason for hiding this comment

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

what are other combinations? if env can be combined

That's a judgment call. The idea here (and in the quip doc) is to figure that out. I think we have decided it by now. The combinations are as we've listed them here (after making the changes mentioned in this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it.

"enum":[
"cloud9",
"codeCatalyst",
"dev",
Copy link
Contributor

Choose a reason for hiding this comment

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

is this for "gamma" or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually i added two words for dev & local that mean the same. removed it

"type":"string",
"enum":[
"cloud9",
"codeCatalyst",
Copy link
Contributor

Choose a reason for hiding this comment

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

why not codecatalyst?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

i do not remember the exact context why this file is in the public repo, but changes need to be backported to the internal .xml file to take effect service-side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dhruvigajjar and others added 3 commits January 10, 2024 10:27
…ub.com:aws/aws-toolkit-common into gajjardh/add-comput-env-field-to-metric-events
@dhruvigajjar dhruvigajjar merged commit 21dea92 into main Jan 16, 2024
7 checks passed
@dhruvigajjar dhruvigajjar deleted the gajjardh/add-comput-env-field-to-metric-events branch January 16, 2024 21:44
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