yurishkuro / opentracing-tutorial

A collection of tutorials for the OpenTracing API
Apache License 2.0
1.57k stars 407 forks source link

go-lesson03: Client instrumentation unclear #18

Closed eest closed 6 years ago

eest commented 6 years ago

Hello again,

Thanks for the quick handling of the previous issues I raised! Here is another issue I noticed: The lesson tells you to instrument the client with this snippet:

ext.SpanKindRPCClient.Set(span)
ext.HTTPUrl.Set(span, url)
ext.HTTPMethod.Set(span, "GET")
span.Tracer().Inject(
    span.Context(),
    opentracing.HTTPHeaders,
    opentracing.HTTPHeadersCarrier(req.Header),
)

The first problem with this is that "ext" is not imported at this point in the example (and its existence is not mentioned). Looking at the solution you can tell an import of "github.com/opentracing/opentracing-go/ext" is missing. You might want to add the equivalent of "Add some imports" as is done when instrumenting the servers further down.

Finally, when the snippet calls ext.HTTPUrl.Set(span, url) there is no url variable in scope, but it is present in the solution like so:

url := "http://localhost:8081/format?" + v.Encode()

Maby the baseline exercise code should be updated to use that variable so you don't have to stop and figure out what "url" is when following along? Since net/url is imported it is even more confusing since you might think "url" refers to that package and not a missing variable.

I really appreciate this tutorial by the way, thanks a lot for putting it out there!

yurishkuro commented 6 years ago

would you like to submit a PR to fix these?

eest commented 6 years ago

@yurishkuro: I have added a PR, let me know what you think.

yurishkuro commented 6 years ago

thanks!