Skip to content

startSpan should take tracepoint location #68

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

Merged
merged 9 commits into from
Nov 7, 2022
Merged

startSpan should take tracepoint location #68

merged 9 commits into from
Nov 7, 2022

Conversation

ktoso
Copy link
Member

@ktoso ktoso commented Oct 6, 2022

resolves #67

This allows the implementation of ad-hoc tracerbullets and configurable per "trace location" tracers.

Here's the gist of it:

        tracer.enableTracepoint(fileID: fileID, line: line)
        // Imagine this is set via some "ops command", e.g. `<control> <pid, or ssh or something> trace enable Sample.swift:1234`
        // Effectively enabling tracepoints is similar to tracer bullets, tho bullets are generally "one off",
        // but here we could attach a trace-rate, so e.g.: control `<pid> trace enable Sample:1234 .2` to set 20% sampling rate etc.

        tracer.withSpan("dont") { span in
            // don't capture this span...
        }
        tracer.withSpan("yes", file: #fileID, line: 7777) { span in
            // do capture this span, and all child spans of it!
            tracer.withSpan("yes-inner") { inner in
                // since the parent of this span was captured, this shall be captured as well
            }
        }

or even better:

tracer.enableTracepoint(function: "example()")

@ktoso
Copy link
Member Author

ktoso commented Oct 6, 2022

Paging @weissi - the idea that we talked about yesterday. this would enable various location sensitive configuration or whatever one would want to do in a tracer

func startSpan(
_ operationName: String,
baggage: Baggage,
ofKind kind: SpanKind,
at time: DispatchWallTime
at time: DispatchWallTime,
Copy link
Member

Choose a reason for hiding this comment

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

why not the new Clock/Duration/... API?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah just noticed this too tbh...

I'll need to think about this, it'd force users to require 5.7+ so we may not want that... we'd want vapor to adopt without having to suddenly bump their required swift versions. Probably we can add an public API accepting Instant but I guess implementors may have to implement against dispatch time or something else... to keep the swift requirement low :-/

I'll think about it more and ticketify. AHC we'd also not want to force to require 5.7 suddenly 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add an overload though that delegates to the dispatch time one 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add overload in separate PR

Copy link
Member Author

Choose a reason for hiding this comment

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

#71

@ktoso ktoso force-pushed the tracepoint-enable branch from b8e4975 to 12d7326 Compare October 6, 2022 11:38
@ktoso
Copy link
Member Author

ktoso commented Nov 7, 2022

@swift-server-bot test this please

@ktoso ktoso force-pushed the tracepoint-enable branch from e7341c3 to e4773fb Compare November 7, 2022 08:18
@ktoso ktoso force-pushed the tracepoint-enable branch from e4773fb to 8ed9307 Compare November 7, 2022 08:24
@ktoso ktoso force-pushed the tracepoint-enable branch 2 times, most recently from 361c8dc to 12b040a Compare November 7, 2022 08:34
@ktoso ktoso force-pushed the tracepoint-enable branch from 12b040a to 7ea80f7 Compare November 7, 2022 08:35
Copy link
Collaborator

@slashmo slashmo left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -47,25 +53,111 @@ public protocol Tracer: Instrument {
}

extension Tracer {
/// Start a new ``Span`` with the given `Baggage` starting at `DispatchWallTime.now()`.
#if swift(>=5.3.0)
/// Start a new ``Span`` with the given `Baggage` starting "now".
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good simplification 👍

ofKind kind: SpanKind = .internal
ofKind kind: SpanKind = .internal,
function: String = #function,
file fileID: String = #fileID,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm very much looking forward to a time where we can start to only support #fileID in server-side Swift projects 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

Tbh NIO requires 5.5+ already, so most projects can. I'm not even sure we should keep trying with 5.1+ here... but for now we do.

@ktoso ktoso merged commit 8f5315e into main Nov 7, 2022
@ktoso ktoso deleted the tracepoint-enable branch November 7, 2022 10:35
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.

startSpan and friends should take source location
3 participants