Opentelemetry-go: Should `Tracer#WithSpan` take options?

[iredelmeier]

[jmacd]

I support the idea in principle, but it feels like it clutters the API too, because you'll have to write a []pkg.Option{} boilerplate to use this. I'd probably rather see calls like

return trace.WithSpan(ctx, trace.SpanName("abc").More(...).Optionals(...).Here(...), func(ctx) error {
    ...
})

[freeformz]

@jmacd You don't have to create a slice to pass var args. WithSpan could be:

    WithSpan(
        ctx context.Context,
        operation string,
        body func(ctx context.Context) error,
        opts ...Option,
    ) error

[freeformz]

This would also make it symmetric to NewHandler and the like.

[jmacd]

I think placing the variable length args after the function destroys
readability.

On Tue, Oct 8, 2019 at 6:18 PM Edward Muller [e-mail]
wrote:

This would also make it symmetric to NewHandler
https://godoc.org/go.opentelemetry.io/plugin/othttp#NewHandler and the
like.

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/open-telemetry/opentelemetry-go/issues/149?email_source=notifications&email_token=AA3WFCJ25IQHIV4HUHLQBRDQNUWMRA5CNFSM4I25NOE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAWE4PI#issuecomment-539774525,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA3WFCLMHYGWYZGRBKAGHV3QNUWMRANCNFSM4I25NOEQ
.

[freeformz]

It's not great, but IMO, it is better than the odd chaining thing.

return trace.WithSpan(ctx,"op",
  func(c context.Context) error {
    ...
  },
  trace.WithMore(...),trace.WithHere(...),
)

Additionally, adding the variable options at the end is compatible with the existing signature.

[jmacd]

It's not bad, I agree. I support this.

It's a bit of a "buried lede", where potentially most important information is a distance away from the start of the statement. I'd accompany this with a recommendation to avoid lengthy in-line func definitions when options are present.

[paivagustavo]

Just throwing another idea, what if we change this a little bit and do something like:

return trace.WithSpan(ctx,"op", trace.WithMore(...),trace.WithHere(...))
    .Do(func(c context.Context) error {
            ...
    })

[freeformz]

I like that.

[jmacd]

What happens if .Do(...) is left off, what type is returned by WithSpan?

[paivagustavo]

Having a hard time thinking of a good name for the intermediate type returned by WithSpan.

edit: I'm not liking this ideia anymore, please ignore.

~I have been thinking about the experience of using the API, would be possible to change add a .Do(...) method to the Span type? With this possible could drop WithSpan altogether (specs doesn't actually forces us to have it, or did I missed it?).~

~trace.StartSpan(ctx, "name", ...).Do(...) seems a very natural way of using the API.~

~I don't know if this is a change that may needs to go to specs?~

[paivagustavo]

I think that SpanScope maybe a good start name if are going to keep this structure.

We could create the span when the Do method is called, so if one does not call Do, nothing happens.

[freeformz]

That is one problem with chaining in that way. As for a type name: SpanDoer (an interface), SpanFuture, SpanScope, etc.

Given that .Do may require another type I'd prefer the inline func, with trailing ...opts.

[iredelmeier]

Any more thoughts on this? It seems like the trailing varargs is most straightforward.

[jmacd]

🤷‍♂ Sure!

[jmacd]

Let's go with trailing varargs.