Skip to content
This repository has been archived by the owner on Apr 23, 2021. It is now read-only.

Reconsider package modularization #31

Closed
slashmo opened this issue Jun 13, 2020 · 10 comments
Closed

Reconsider package modularization #31

slashmo opened this issue Jun 13, 2020 · 10 comments
Assignees
Projects
Milestone

Comments

@slashmo
Copy link
Owner

slashmo commented Jun 13, 2020

https://forums.swift.org/t/server-distributed-tracing/37464/26

As BaggageContext and Instrumentation are closely related, does it make sense to package them up in one library instead of the two we currently have?

@slashmo
Copy link
Owner Author

slashmo commented Jun 25, 2020

For now, we'll keep both libraries separate as we plan on adding BaggageContext as a dependency to NIO and want to keep it as small as possible. Also, everything instrumentation related in NIO could probably be done in a separate project/repo.

@slashmo slashmo added this to To do in Development Jun 29, 2020
@slashmo
Copy link
Owner Author

slashmo commented Jul 14, 2020

On the contrary, we may want to consider extracting TracingInstrument & related types into a separate library as well, as we may end up with multiple of such tool-specific types/logic for a variety of cross-cutting tools, which could start to bloat the Instrumentation library.

@pokryfka
Copy link
Contributor

pokryfka commented Jul 22, 2020

for a starter, maybe moving TracingInstrument to its of target within the same package may be a good approach (?)
that way it will be trivial to move it to another Package if practical (for example lots of different types and implementations of different "instruments")

currently there are:

let package = Package(
    name: "gsoc-swift-tracing",
    products: [
        .library(name: "BaggageLogging", targets: ["BaggageLogging"]),
        .library(name: "Instrumentation", targets: ["Instrumentation"]),
        .library(name: "NIOInstrumentation", targets: ["NIOInstrumentation"])
    ],

@ktoso
Copy link
Collaborator

ktoso commented Jul 22, 2020

Yeah I've been floating that idea around since a while;

The open question is naming, what should it be;

  • Instrumentation & TracingInstrumentation

is what I'm warming up towards... Not sure if Baggage should be in the names, like BaggageInstrumentation or not.

@pokryfka
Copy link
Contributor

pokryfka commented Jul 22, 2020

related question is how the InstrumentationSystem should be configured,
for instance should there be only one instrument, one of a kind and so on:

    // FIXME: smarter impl
    public static var tracer: TracingInstrument {
        self.lock.withReaderLock {
            let tracer: TracingInstrument? = self._tracer
            let res: TracingInstrument = tracer ?? NoOpTracingInstrument()
            return res
        }
    }

BTW the code above will not find a tracer if its provided by official and public MultiplexInstrument

@ktoso
Copy link
Collaborator

ktoso commented Jul 22, 2020

Yes that's not solved yet; Let's not conflate tickets and discuss that in here: #86

@slashmo
Copy link
Owner Author

slashmo commented Jul 25, 2020

Instrumentation & TracingInstrumentation
is what I'm warming up towards... Not sure if Baggage should be in the names, like BaggageInstrumentation or not.
@ktoso

I think the Baggage prefix here could be a bit misleading as we’re not instrumenting the Baggage itself but cross-cutting tools, using BaggageContext for propagation. That being said, something like CrossCuttingInstrumentation is a bit too verbose in my opinion.

@ktoso
Copy link
Collaborator

ktoso commented Jul 27, 2020

CrossCuttingInstrumentation

That's too many "new words", we can quickly teach about "there's some baggage context thing" and "there's some instrumentation (that uses it)". I don't want to have to dive deep into why we're doing the separation of concerns just for someone who's in a rush trying to get a job done -- the separation and cross cutting wording I do want to keep but in documentation and rationale why we're separating things as we are.

Instrumentation & TracingInstrumentation

I guess that's okey (unless someone comes up with that we're using up those names aggressively), we could use Baggage prefix if necessary.

@ktoso
Copy link
Collaborator

ktoso commented Jul 27, 2020

BTW the code above will not find a tracer if its provided by official and public MultiplexInstrument

@pokryfka That's why it has a FIXME 😉 PoCs welcome how we should implement those instrument getters.

I ticketified this explicitly now: Solve "getting" (specific) tracers from instrumentation system #99

@ktoso
Copy link
Collaborator

ktoso commented Oct 9, 2020

We figured out the modularization with teams internally, closing for now.

repos:

  • baggage core
  • baggage
  • tracing

@ktoso ktoso closed this as completed Oct 9, 2020
Development automation moved this from To do to Done Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development
  
Done
Development

No branches or pull requests

3 participants