-
Notifications
You must be signed in to change notification settings - Fork 23
[docs] organization, style, and grammar updates #81
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
Conversation
…t tense, and american english spelling choices. Fixes some typos as well.
…nix -> UNIX, rework a few initializer docs that aren't yet public
czechboy0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @heckj, added a few more suggestions
| @@ -0,0 +1,13 @@ | |||
| # ``SystemMetricsMonitor`` | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been meaning to ask - what's the docc command to generate these? Or did you hand-write them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol - I hand wrote these. There's not a DocC command to generate them one off, unfortunately- I've just got 'my pattern' of how I do it, which it mostly stub, see what show's up in preview, then iterate to drop the right pieces into place.
That said, there is a curation generation command - although I'm not certain if anyone actually uses it, as it's not detailed anywhere in the docs. The gist of which is:
- create/get the symbols for the target/module
- use
docc process-catalogto emit a default catalog
mkdir .symbolgraphs
xcrun swift build --target MyProjectExample \
-Xswiftc -emit-symbol-graph \
-Xswiftc -emit-symbol-graph-dir \
-Xswiftc .symbolgraphs
mkdir Sources/MyProjectExample/Documentation.docc
# better generation using --from-symbol
# which constrains the generated content to MyProjectExample
xcrun docc process-catalog \
emit-generated-curation \
Sources/MyProjectExample/Documentation.docc \
--from-symbol MyProjectExample \
--additional-symbol-graph-dir .symbolgraphs
So it's kind of a pain to use...
| ## Topics | ||
|
|
||
| ## Getting started | ||
| ### Monitor system metrics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ### Monitor system metrics | |
| ### Essentials |
I think "Essentials" is the usual name of the first section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We see it commonly, but at least in the broader API guides, the last guidance I got here was essentials was meant to be "the things you critically had to know to be able to use the API", which I took as a placeholder to mean explanatory content prefixed into the getting started pieces when needed. This didn't quite seem to hit that bar for me, so I leaned into just giving a functional heading that made it reinforced what this was doing, especially since this is just a simple API surface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's fair - though I'd argue if you don't learn the SystemMetricMonitor API, then there isn't much you can do with the library. So IMO it does raise to the bar of being essential.
| ## Overview | ||
|
|
||
| ``SystemMetricsMonitor`` automatically collects key process metrics and reports them through the Swift Metrics API. | ||
| Create an instance of ``SystemMetricsMonitor`` to automatically collect key process metrics and report them through the Swift Metrics API. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should preserve the short code snippet, including the dependency snippets, here as well. We try to do that in most of our other packages, as that's what most people landing on this page want to see. We should still preserve the new, more detailed getting started guide, though.
So, I think this should show up on this page (the least customized way to run a system metrics monitor):
import SystemMetrics
import ServiceLifecycle
import Metrics
import Logging
@main
struct Application {
static func main() async throws {
let logger = Logger(label: "Application")
let metrics = MyMetricsBackendImplementation()
MetricsSystem.bootstrap(metrics)
let service = FooService()
let systemMetricsMonitor = SystemMetricsMonitor(logger: logger)
let serviceGroup = ServiceGroup(
services: [service, systemMetricsMonitor],
gracefulShutdownSignals: [.sigint],
cancellationSignals: [.sigterm],
logger: logger
)
try await serviceGroup.run()
}
}
README.md
Outdated
| import SystemMetrics | ||
|
|
||
| // Create a logger, or use one of the existing loggers | ||
| let logger = Logger(label: "MyLogger") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add the metrics factory here as well to avoid confusion about where the metrics go, so:
import SystemMetrics
import ServiceLifecycle
import Metrics
import Logging
@main
struct Application {
static func main() async throws {
let logger = Logger(label: "Application")
let metrics = MyMetricsBackendImplementation()
MetricsSystem.bootstrap(metrics)
let service = FooService()
let systemMetricsMonitor = SystemMetricsMonitor(logger: logger)
let serviceGroup = ServiceGroup(
services: [service, systemMetricsMonitor],
gracefulShutdownSignals: [.sigint],
cancellationSignals: [.sigterm],
logger: logger
)
try await serviceGroup.run()
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in a follow on PR, but this example (which I copy-pasted) isn't using the factory initializer - wasn't 100% sure what that looked like, and honestly if we wanted to promote that path or not. I expect that anything in the README will be the first thing to be copy/pasted into place as an example that gets modified - so I think whatever we expect to be most commonly used would be best here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@czechboy0 do you suggest we change example to use SystemMetricsMonitor(metricsFactory:logger:)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should show the simplest one in the README, so the one without the factory parameter. Customizing the metrics factory is a slightly more advanced use case.
kukushechkin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for these improvements and explaining the reasoning behind it.
Co-authored-by: Honza Dvorsky <[email protected]>
Co-authored-by: Honza Dvorsky <[email protected]>
| Create an instance of ``SystemMetricsMonitor`` to automatically collect key process metrics and report them through the Swift Metrics API. | ||
|
|
||
| ### Available metrics | ||
| The monitor collects the following metrics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can maybe move the constants into a nested bullet point under each metric? That way they'll always start on the same column.

Motivation:
Review of content against docs style guides for upcoming 1.0 release
Modifications:
Before:
After:
To quickly preview locally:
ex:

Biggest notable change that you we may not want to run with - I trimmed down what was at the top level, shifting away from the format that swift-configuration used, because that information felt overwhelming (it felt like a giant README trying to shove too much in there), and moved the getting started into a concise article that I curated inline.
I haven't added, but could, a quick walk through article with image snapshots w/
Examples/ServiceIntegrationto show setting it up and seeing the metrics being reported in a local grafana instance, although I think the README in that directory is already pretty darned good and gets to the core for someone who just wants to see it work to get a sense of what's included.