-
Notifications
You must be signed in to change notification settings - Fork 33
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
[Docs] Combined initial documentation work #121
Conversation
// | ||
// This source file is part of the Swift OpenTelemetry open source project | ||
// | ||
// Copyright (c) 2021 Moritz Lang and the Swift OpenTelemetry project authors |
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.
Authorship I believe you'd be fine with the samples using this repo's copyright @slashmo ?
I'll change that in a bit
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 fine 👍
Update Samples/Dinner/docker/collector-config.yaml Co-authored-by: Moritz Lang <[email protected]> minor cleanup wip
**Motivation:** Since modifying an attribute needs to hit a lock this pattern is more efficient
**Motivation:** This type will be deprecated as soon as possible and only serves as a bridge for Swift 5.6 adopters.
swift run -c release | ||
``` | ||
|
||
Refer to the "Trace Your Application" guide in the documentation to learn more about how to interpret this sample. |
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.
Could this link to "Trace Your Application" guide directly from 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.
Yeah okey
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.
Heh actually such link is not possible unless as a <a href... I can do those later, I don't know the URL to point to unless we have something published on the package index.
Co-authored-by: Max Desiatov <[email protected]>
Co-authored-by: Yim Lee <[email protected]>
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 went through part of the application example and ran into the fact that it seems like the example is already the parallelized trace, despite what the docs suggest. Is that expected? It seems like it might be nice to have it be able to produce both kinds of traces so that someone following the tutorial can actually explore the comparison features, etc.
> ``` | ||
> | ||
> - Zipkin UI URL: http://localhost:9411/ | ||
> - Jaeger UI URL: ... |
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.
This space unintentionally left blank? Partially duplicates the content above, but also this is the spot where I looked.
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 I'll remove those -
here, we mention it above right
Samples/Dinner/Package.swift
Outdated
let package = Package( | ||
name: "onboarding", | ||
platforms: [ | ||
.macOS("14.0.0"), |
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.
Should this really be 14.0.0? I adjusted this to 13.0.0 so I could run it and it worked fine.
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'll drop to 13 thanks
**Motivation:** This type will be deprecated as soon as possible and only serves as a bridge for Swift 5.6 adopters.
I take the few minor comments as overall approval, thanks folks! |
Co-authored-by: Cassie Jones <[email protected]>
I think that's off and I'll follow up on that, thank you @porglezomp ! #124 |
} | ||
|
||
func chopVegetables() async throws -> [Vegetable] { | ||
try await otelChopping1.tracer().withSpan("chopVegetables") { _ in |
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 know I'm coming in late, but how come this is explicitly referencing otelChopping1
?
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.
Just to mock a multi node trace in one process. I'll make an actual multi process example soon to complement this with more "normal" usage
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.
Oh, got it. That wasn't explicitly clear when looking at this example what it was actually demonstrating
|
||
`Instrument` has two requirements: | ||
|
||
1. An `Instrument/extract(_:into:using:)` method, which extracts values from a generic carrier (e.g. HTTP headers) and store them into a `Baggage` instance |
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'm assuming this is referenced this way, so that when cross-module symbols are able to be referenced by DocC, we can just come back and add the 2nd set of `?
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 currently we can't cross link so it's just a code marker. I may go around and make them explicit links manually hmm
@@ -27,4 +26,4 @@ | |||
## | |||
##===----------------------------------------------------------------------===## | |||
|
|||
swift package --disable-sandbox preview-documentation --target $1 | |||
xcrun swift package --disable-sandbox preview-documentation --target Tracing |
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.
Well this doesn't seem that portable 😛
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.
What would be the alternative? Is there a portable way to pick up the currently selected toolchain? I don't think non-Darwin platforms even allow something like that.
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.
Platform checks to determine if it should be prefixed with xcrun
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.
Potentially, I'm not sure if the intention was to create a script that runs purely on macOS CI.
Replaces #69
Resolves #80