Skip to content

Commit 0b63c71

Browse files
authored
docs #4079
1 parent ff2882c commit 0b63c71

File tree

6 files changed

+144
-46
lines changed

6 files changed

+144
-46
lines changed

CONTRIBUTING.md

Lines changed: 41 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -81,9 +81,28 @@ You can also use these NPM tasks (see `npm run` for the full list):
8181
8282
- Project patterns and practices: [CODE_GUIDELINES.md](./docs/CODE_GUIDELINES.md)
8383
- [VS Code Extension Guidelines](https://code.visualstudio.com/api/references/extension-guidelines)
84+
- [Webview guidance](https://code.visualstudio.com/api/ux-guidelines/webviews)
8485
- [VS Code API Documentation](https://code.visualstudio.com/api/references/vscode-api)
8586
- [VS Code Extension Capabilities](https://code.visualstudio.com/api/extension-capabilities/common-capabilities)
8687
88+
### Prerelease artifacts
89+
90+
- CI automatically publishes GitHub [prereleases](https://github.com/aws/aws-toolkit-vscode/releases)
91+
for `master` and `feature/x` branches, including `.vsix` artifacts which can
92+
be used to test the latest build for that branch. Each prerelease and its
93+
artifact are continually updated from the HEAD of its branch.
94+
- PR artifacts: each pull request is processed by an AWS CodeBuild job which
95+
runs all tests and provides the build result via the _Details_ link as shown
96+
below.
97+
- <img src="./docs/images/ci-artifact.png" alt="CI artifact" width="512"/>
98+
99+
### Debug failing integration tests
100+
101+
- Check for recent changes in each of these projects:
102+
- https://github.com/microsoft/vscode-python (releases)
103+
- https://github.com/aws/aws-sam-cli/releases
104+
- https://github.com/aws/aws-sam-cli-app-templates/ (`master` branch, not releases!)
105+
87106
### Technical notes
88107
89108
- VSCode extensions have a [100MB](https://github.com/Microsoft/vscode-vsce/issues/179) file size limit.
@@ -170,13 +189,6 @@ To run a single test in VSCode, do any one of:
170189
rootTestsPath: __dirname + '/shared/sam/debugger/'
171190
```
172191
173-
### Debug failing integration tests
174-
175-
- Check for recent changes in each of these projects:
176-
- https://github.com/microsoft/vscode-python (releases)
177-
- https://github.com/aws/aws-sam-cli/releases
178-
- https://github.com/aws/aws-sam-cli-app-templates/ (`master` branch, not releases!)
179-
180192
### Browser Support
181193
182194
Running the extension in the browser (eg: [vscode.dev](https://vscode.dev/)).
@@ -192,17 +204,6 @@ You can find the coverage report at `./coverage/index.html` after running the te
192204
- Exercise the code (`Extension Tests`, `Integration Tests`, etc.)
193205
- Generate a report with `npm run report`
194206
195-
### Prerelease artifacts
196-
197-
- CI automatically publishes GitHub [prereleases](https://github.com/aws/aws-toolkit-vscode/releases)
198-
for `master` and `feature/x` branches, including `.vsix` artifacts which can
199-
be used to test the latest build for that branch. Each prerelease and its
200-
artifact are continually updated from the HEAD of its branch.
201-
- PR artifacts: each pull request is processed by an AWS CodeBuild job which
202-
runs all tests and provides the build result via the _Details_ link as shown
203-
below.
204-
- <img src="./docs/images/ci-artifact.png" alt="CI artifact" width="512"/>
205-
206207
### CodeCatalyst Blueprints
207208
208209
You can find documentation to create VSCode IDE settings for CodeCatalyst blueprints at [docs/vscode-config.md](./docs/vscode-config.md).
@@ -221,11 +222,28 @@ To send a pull request:
221222
2. Modify the source; focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
222223
- Read the [project guidelines](#guidelines), this is very important for non-trivial changes.
223224
3. Commit to your fork [using clear commit messages](#commit-messages).
224-
4. Update the changelog by running `npm run newChange`.
225-
- Note: the main purpose of the `newChange` task is to avoid merge conflicts.
225+
4. Update the [changelog](#changelog).
226226
5. [Create a pull request](https://help.github.com/articles/creating-a-pull-request/).
227227
6. Pay attention to any CI failures reported in the pull request.
228228
229+
### Changelog
230+
231+
Pull requests that change customer-facing behavior (in any _describable_ way) should include
232+
a changelog item(s). Update the changelog via:
233+
234+
npm run newChange
235+
236+
Guidelines:
237+
238+
- Describe the change in a way that is meaningful to the _customer_.
239+
- ❌ `Remove the cache when the connection wizard is re-launched`
240+
- ✅ `Connection wizard sometimes shows the old (stale) connection`
241+
- If there are multiple unrelated changes, run `npm run newChange` for each change.
242+
- **Bug Fix** changes should describe the _problem being fixed_. This tends to produce simpler,
243+
more-readable descriptions. It's redundant to mention "Fixed" in the description. Example:
244+
- ❌ `Fixed S3 bug which caused filenames to be uppercase`
245+
- ✅ `S3 filenames are always uppercase`
246+
229247
### Commit messages
230248
231249
Generally your PR description should be a copy-paste of your commit message(s).
@@ -238,7 +256,6 @@ Follow these [commit message guidelines](https://cbea.ms/git-commit/):
238256
- Imperative voice ("Fix bug", not "Fixed"/"Fixes"/"Fixing").
239257
- Body: for non-trivial or uncommon changes, explain your motivation for the
240258
change and contrast your implementation with previous behavior.
241-
242259
- Often you can save a _lot_ of words by using this simple template:
243260
```
244261
Problem: …
@@ -306,9 +323,9 @@ Example:
306323
```json
307324
"aws.dev.codecatalystService": {
308325
"region": "us-west-2",
309-
"endpoint": "https://public.codecatalyst-gamma.global.api.aws",
310-
"hostname": "integ.stage.REMOVE.codes",
311-
"gitHostname": "git.gamma.source.caws.REMOVE",
326+
"endpoint": "https://codecatalyst-gamma.example.com",
327+
"hostname": "integ.stage.example.com",
328+
"gitHostname": "git.gamma.source.example.com",
312329
}
313330
```
314331

docs/ARCHITECTURE.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@ Complex programs often require more than just simple functions to act as entry-p
8686

8787
## Exceptions
8888

89+
_See also [CODE_GUIDELINES.md](./CODE_GUIDELINES.md#exceptions)._
90+
8991
Large applications often have a correspondingly large number of failure points. For feature-level logic, these failures are often non-recoverable. The best we can do is show the user that something went wrong and maybe offer guidance on how to fix it.
9092

9193
Because this is such a common pattern, shared error handling logic is defined by `ToolkitError` found [here](../src/shared/errors.ts). This class provides the basic structure for errors throughout the Toolkit.

docs/CODE_GUIDELINES.md

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,27 @@ that cannot be enforced by a `lint` build-task.
1111
instead of inventing new conventions.
1212
- Convention: provide global editor commands as an alternative to browsing items in the Explorer.
1313
- Instead of needing to visit service _Foo_ in the Explorer to _View_ its items, consider also providing a `AWS: Foo: View Item` command.
14+
- [Webview guidance](https://code.visualstudio.com/api/ux-guidelines/webviews)
15+
- Webview costs:
16+
- Webviews very easily lead to the [inner-platform effect](https://en.wikipedia.org/wiki/Inner-platform_effect).
17+
Because they are fully isolated from vscode and its extensions, they must include any code
18+
and frameworks.
19+
- Leads to extra dependencies.
20+
- Webviews inherit none of vscode's standard features. This means features like keyboard
21+
shortcuts, syntax highlighting, and editor navigation, are not available to users. Instead
22+
users must learn the custom _web application_ embedded in the webview.
23+
24+
## Dependencies
25+
26+
Dependencies can be very high-leverage if they solve a difficult problem.
27+
Dependencies are also a [maintenance burden](https://github.com/aws/aws-toolkit-vscode/pulls?q=is%3Apr+author%3Aapp%2Fdependabot+is%3Aclosed) and security risk
28+
Copy-pasting or "inlining" a dependency doesn't solve that, of course--it only hides the problem (another cost).
29+
So before taking on a new dependency, ask:
30+
31+
- is this solving a problem that is worth the cost?
32+
- could the problem be solved in some other way that involves a smaller cost? For example, using
33+
an isolated function with good test coverage, or a native vscode feature such as a TreeView or
34+
quickpick menu.
1435

1536
## Naming
1637

@@ -91,6 +112,13 @@ that is a net cost.
91112
- Localize UI messages. Do _not_ localize log and exception messages.
92113
- Use `extensionUtilities.getIdeProperties()` to automatically match IDE
93114
terminology (e.g. VS Code : CodeLens :: AWS Cloud9 : Inline Action)
115+
- Refactoring tools allow us to avoid "premature abstraction". Avoid wrappers
116+
or other abstractions until the need is clear and obvious.
117+
118+
### Exceptions
119+
120+
_See also [ARCHITECTURE.md](./ARCHITECTURE.md#exceptions)._
121+
94122
- Bubble-up error conditions, do not sink them to random hidden places (such as
95123
logs only), expecting callers to figure out the failure mode. If a caller
96124
spawns a process that fails, the caller should get an exception, callback, or
@@ -101,10 +129,22 @@ that is a net cost.
101129
attaching metadata to the error, or retrying the failed action. Do not just
102130
log and rethrow the error without additional context as to where the error occured.
103131
- Do not log a stack trace unless it's truly a fatal exception. Stack traces are
104-
noisy and mostly useless in production because the extension is 'bundled', removing
105-
anything useful from the trace.
106-
- Refactoring tools allow us to avoid "premature abstraction". Avoid wrappers
107-
until the need is clear and obvious.
132+
noisy and mostly useless in production because the extension is bundled
133+
(webpacked), removing anything useful from the trace.
134+
- When _debugging_ the extension, you can tell the debugger to break on
135+
exceptions, so logging the stacktrace is unnecessary there.
136+
- Do not use multiple logger calls to log what is semantically a single
137+
message. Use a string template or printf-style syntax (`%s` ) to format the
138+
message:
139+
- GOOD:
140+
```
141+
getLogger().error(`Failed to create %s: %s`, foo, (err as Error).message)
142+
```
143+
- BAD:
144+
```
145+
getLogger().error(`Failed to create %s`, foo)
146+
getLogger().error(err)
147+
```
108148
109149
## Test guidelines
110150

docs/develop.md

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,22 @@ Notes about the codebase, its utilities, special globals, etc.
55
## VSCode context keys
66

77
VScode extensions can use vscode 'setContext' command to set special context keys which are
8-
available in `package.json`. This extension sets the following keys:
8+
available in `package.json`.
99

10+
### Defining a new setContext key
11+
12+
If you must define a new key,
13+
14+
- Prefix it with `aws.` as recommended by the [vscode docs](https://code.visualstudio.com/api/extension-guides/command#using-a-custom-when-clause-context).
15+
- Use brevity. Less is more.
16+
- Document it in the list below.
17+
18+
### Toolkit setContext keys
19+
20+
We set the following keys:
21+
22+
- `isCloud9`: This is hardcoded by Cloud9 itself, not the Toolkit.
23+
- Cloud9 _does not support setContext_. So this is the only usable key there.
1024
- `aws.codecatalyst.connected`: CodeCatalyst connection is active.
1125
- `CODEWHISPERER_ENABLED`: CodeWhisperer connection is active.
1226
- `aws.isDevMode`: AWS Toolkit is running in "developer mode".

docs/telemetry.md

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,35 @@
11
# Telemetry
22

3+
## Development
4+
35
See [aws-toolkit-common/telemetry](https://github.com/aws/aws-toolkit-common/tree/main/telemetry#telemetry) for full details about defining telemetry metrics.
46

57
- You can define new metrics during development by adding items to
68
[telemetry/vscodeTelemetry.json](https://github.com/aws/aws-toolkit-vscode/blob/21ca0fca26d677f105caef81de2638b2e4796804/src/shared/telemetry/vscodeTelemetry.json).
7-
- Building the project will trigger the `generateClients` build task, which generates new symbols in `shared/telemetry/telemetry`, which you can import via:
9+
- The `generateClients` build task generates new symbols in `shared/telemetry/telemetry`, which you can import via:
810
```
9-
import { telemetry } from '../../shared/telemetry/telemetry'
11+
import { telemetry } from '/shared/telemetry/telemetry'
1012
```
11-
- The metrics defined in `vscodeTelemetry.json` should be upstreamed to [aws-toolkit-common](https://github.com/aws/aws-toolkit-common/blob/main/telemetry/definitions/commonDefinitions.json) after launch (at the latest).
13+
- When your feature is released, the "development" metrics you defined in `vscodeTelemetry.json` should be upstreamed to [aws-toolkit-common](https://github.com/aws/aws-toolkit-common/blob/main/telemetry/definitions/commonDefinitions.json).
1214
- Metrics are dropped (not posted to the service) if the extension is running in [CI or other
1315
automation tasks](https://github.com/aws/aws-toolkit-vscode/blob/21ca0fca26d677f105caef81de2638b2e4796804/src/shared/vscode/env.ts#L71-L73).
1416
- You can always _test_ telemetry via [assertTelemetry()](https://github.com/aws/aws-toolkit-vscode/blob/21ca0fca26d677f105caef81de2638b2e4796804/src/test/testUtil.ts#L164), regardless of the current environment.
1517
16-
### Incrementally Building a Metric
18+
## Guidelines
19+
20+
- Use `run()` where possible. It automatically sets the `result` and `reason` fields. See below for details.
21+
- `run()` gets the `reason` value from the `Error.code` property of any exception that is thrown.
22+
- Your code can throw `ToolkitError` with a `code` field to communicate errors, validation issues, or [cancellation](https://github.com/aws/aws-toolkit-vscode/blob/06661f84530c6b5331579871d685515700b7767e/src/auth/sso/model.ts#L138). See below.
23+
- The `reason` and `result` fields are standard metric fields shared by all Toolkits (VSCode, JetBrains, VisualStudio).
24+
They should be used instead of special-purpose metrics or fields.
25+
- `result` allows the Toolkits team to monitor all features for potential regressions.
26+
- `reason` gives insight into the cause of a `result=Failed` metric.
27+
- `telemetry.record()` called during a `telemetry.foo.run(…)` context will automatically annotate the current `foo` metric.
28+
- For example, the cloudwatch logs feature adds `hasTimeFilter` info its metrics by [calling telemetry.record()](https://github.com/aws/aws-toolkit-vscode/blob/06661f84530c6b5331579871d685515700b7767e/src/cloudWatchLogs/cloudWatchLogsUtils.ts#L21-L24).
1729
18-
In certain scenarios, you may have some code that has multiple stages/steps in its execution.
30+
## Incrementally Building a Metric
31+
32+
User actions or other features may have multiple stages/steps, called a "workflow" or just "flow". A telemetry ["trace"](https://opentelemetry.io/docs/concepts/signals/traces/) captures a flow as tree of ["spans"](https://opentelemetry.io/docs/concepts/signals/traces/#spans).
1933
2034
For example, `setupThing()` has multiple steps until it is completed, ending with `lastSetupStep()`.
2135
@@ -49,12 +63,24 @@ Here we emitted a final metric based on the failure or success of the entire exe
4963

5064
<br>
5165

52-
But usually code is not flat and there are many nested calls. If something goes wrong during the execution it would be useful to have more specific information at the area of failure. So what we can do is use `run()` along with `record()`.
66+
But usually code is not flat and there are many nested calls. If something goes wrong during the execution it would be useful to have more specific information at the area of failure. For that we can use `run()` along with `telemetry.record()`.
67+
68+
`run()` accepts a callback, and when the callback is executed, any uses of `telemetry.record()` at _any nesting level_ during execution of that callback, will update the
69+
attributes of the ["current metric"](https://github.com/aws/aws-toolkit-vscode/blob/13cb98d5315092ddc9eb5ba898e5f26810dada25/src/shared/telemetry/spans.ts#L233).
70+
And at the end (that is, when `run()` returns) we will emit a single metric with the last updated attributes.
71+
[Example](https://github.com/aws/aws-toolkit-vscode/blob/06661f84530c6b5331579871d685515700b7767e/src/cloudWatchLogs/cloudWatchLogsUtils.ts#L21-L24)
72+
73+
When an exception is thrown from a `run()` context, `run()` will [automatically set](https://github.com/aws/aws-toolkit-vscode/blob/a583825bec6cb68c4942fa60d185644833528532/src/shared/errors.ts#L273-L289)
74+
the `reason` field based on the Error `code` field. You can explicitly set `code` when throwing
75+
a `ToolkitError`, for [example](https://github.com/aws/aws-toolkit-vscode/blob/d08e59952a6c75a5c6c00fdc464e26750c0e85f5/src/auth/auth.ts#L530):
76+
77+
throw new ToolkitError('No sso-session name found in ~/.aws/config', { code: 'NoSsoSessionName' })
78+
79+
Note: prefer reason codes with a format similar to existing codes (not sentences). You can find existing codes by searching the codebase:
5380

54-
`run()` takes in a callable, and when the callable is executed, any uses of `record()` within that callable will update the
55-
attributes of the specific metric. And at the end we will emit a single metric with the last updated attributes.
81+
git grep 'code: '
5682

57-
For example:
83+
### Example
5884

5985
```typescript
6086
setupThing()

src/shared/telemetry/spans.ts

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,6 @@ import {
1818
import { getTelemetryReason, getTelemetryResult } from '../errors'
1919
import { entries, NumericKeys } from '../utilities/tsUtils'
2020

21-
/**
22-
*
23-
*
24-
* For information on how to use "span" related code see docs/telemetry.md
25-
*
26-
*
27-
*/
28-
2921
const AsyncLocalStorage: typeof AsyncLocalStorageClass =
3022
require('async_hooks').AsyncLocalStorage ??
3123
class<T> {
@@ -92,6 +84,13 @@ function getValidatedState(state: Partial<MetricBase>, definition: MetricDefinit
9284
return missingFields.length !== 0 ? Object.assign({ missingFields }, state) : state
9385
}
9486

87+
/**
88+
* A span represents a "unit of work" captured for logging/telemetry.
89+
* It can contain other spans recursively, then it's called a "trace" or "flow".
90+
* https://opentelemetry.io/docs/concepts/signals/traces/
91+
*
92+
* See also: docs/telemetry.md
93+
*/
9594
export class TelemetrySpan<T extends MetricBase = MetricBase> {
9695
#startTime?: Date
9796

0 commit comments

Comments
 (0)