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

Adds tracing documentation #1039

Merged
merged 4 commits into from
Dec 30, 2024
Merged

Adds tracing documentation #1039

merged 4 commits into from
Dec 30, 2024

Conversation

NeedleInAJayStack
Copy link
Contributor

Adds new Tracing chapter to document API introduced in vapor/vapor#3253

app.get("fetchAndProcessNIO") { req in
withSpan("fetch", context: req.serviceContext) { span in
fetchSomething().map { result in
withSpan("process", context: span) { _ in
Copy link

Choose a reason for hiding this comment

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

Suggested change
withSpan("process", context: span) { _ in
withSpan("process", context: span.context) { _ in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch! I've fixed it. Thanks!

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Couple of comments and need to move the location

Copy link
Member

Choose a reason for hiding this comment

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

Let's move this to advanced

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved it to the advanced section.


Vapor's tracing API is built on top of [swift-distributed-tracing](https://github.com/apple/swift-distributed-tracing), which means it is compatible with all of swift-distributed-tracing's [backend implementations](https://github.com/apple/swift-distributed-tracing/blob/main/README.md#tracing-backends).

If unfamiliar with tracing and spans in Swift, review the [OpenTelemetry Trace documentation](https://opentelemetry.io/docs/concepts/signals/traces/) and [swift-distributed-tracing documentation](https://swiftpackageindex.com/apple/swift-distributed-tracing/main/documentation/tracing).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If unfamiliar with tracing and spans in Swift, review the [OpenTelemetry Trace documentation](https://opentelemetry.io/docs/concepts/signals/traces/) and [swift-distributed-tracing documentation](https://swiftpackageindex.com/apple/swift-distributed-tracing/main/documentation/tracing).
If you are unfamiliar with tracing and spans in Swift, review the [OpenTelemetry Trace documentation](https://opentelemetry.io/docs/concepts/signals/traces/) and [swift-distributed-tracing documentation](https://swiftpackageindex.com/apple/swift-distributed-tracing/main/documentation/tracing).


## TracingMiddleware

To automatically create a fully annotated span for each request, add the `TracingMiddleware` to your application.
Copy link
Member

Choose a reason for hiding this comment

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

We should add a note about this being added before any middleware that will make requests to external services

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done!

Copy link
Contributor Author

@NeedleInAJayStack NeedleInAJayStack left a comment

Choose a reason for hiding this comment

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

Thanks for the review! I think I've addressed all the comments.


## TracingMiddleware

To automatically create a fully annotated span for each request, add the `TracingMiddleware` to your application.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved it to the advanced section.

app.middleware.use(TracingMiddleware())
```

To ensure that tracing identifiers are passed along correctly, this should be added before any middleware that calls tracing APIs or connects to external services.
Copy link

Choose a reason for hiding this comment

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

I'd go a step further and say that TracingMiddleware should be as early as possible in the chain, because the sooner this is invoked the more honest is the duration of the span in relation to actual response time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. I've adjusted it to say that it should be added as early as possible.

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

Thanks!

@0xTim 0xTim merged commit a0d7615 into vapor:main Dec 30, 2024
1 check passed
@penny-for-vapor penny-for-vapor bot mentioned this pull request Dec 30, 2024
9 tasks
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.

3 participants