-
Notifications
You must be signed in to change notification settings - Fork 174
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
Document trace API #581
Document trace API #581
Conversation
Gerald Rule: Copy Observability on Veneur, Unilog, Falconer pull requestscc @stripe/observability |
0c1cf0f
to
2a12a97
Compare
(Extracted the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
trace/doc.go
Outdated
// | ||
// The Veneur tracing API is completely independent of the opentracing | ||
// compatibility layer, with the exception of one convenience | ||
// function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to link to or mention the convenience function for completion.
trace/doc.go
Outdated
// Client. Typical applications will want to use the DefaultClient | ||
// exposed by this package. By default, it is set up to send spans to | ||
// veneur listening on the default SSF UDP port, 8128. Client code can | ||
// use SetDefaultClient to change the default client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that some use cases are listed below on L#77. What do you think about moving this last sentence to the end of that paragraph?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not sure it would fit very well there... I've rephrased this para a bit, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. To clarify I'm suggest consolidating the information related to why and how to set a default client. Having 2 separate bits of related information seems like it could be better organized together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think I want to say two things:
- Most applications should set a default client and use that.
- If someone wants to test what traces their app is sending, they need to keep handing around a concrete Client that can be substituted by test code; if so, they'll want to make use of a non-networked backend like the one we expose in veneur/trace/testbackend, a package for testing tracing behavior #582.
This and #582 kinda depend on each other, so I think we'll need to polish this section once more once both have landed.
I had 2 comments but otherwise these changes look good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few suggestions!
Thanks for doing this documentation work! I think this is a great improvement, and long overdue. I left some suggestions inline. |
I think that should be all of them, ready for the old PTAL! |
5ff2618
to
df3fea2
Compare
Updated to the latest master & updated some docs passages to reflect the presence of the PTAL, @aditya-stripe (: |
lgtm |
This should outline most of the things you can use to report traces and metrics with this package.
Also, mention that nil trace clients are fine.
* Mention that they're about the stdlib * Rephrase the intro para so people expect that we talk about Contexts * Get rid of "parented to"
* Clarify when one would serialize spans * Talk about the networked backends' serialization formats
This should be closer to what I wanted to convey (:
f8abd01
to
0ac9ee9
Compare
Rebased to post-10.0 master; the only difference is the changelog entry, so self-approving. |
* master: Add benchmark for protocol.ValidTrace (stripe#692) Report objective metric for spans (stripe#690) Don't log X-Ray write errors at Error level (stripe#689) Ensure SSF packets contain a span or metric before passing to sinks (stripe#688) Add OT trace information on outgoing HTTP reqs (stripe#685) Fix sfx submissions reporting a timeout when one encounters an error (stripe#684) Open CHANGELOG for v12.0.0 (stripe#683) Update CHANGELOG (stripe#682) Update base images for public Docker images to Go 1.11.4 (stripe#681) Allow configuring SFX flush batch size (stripe#680) Update team members on contributing page (stripe#679) Support tag filters in Datadog (stripe#595) Update gRPC, x/sys and protobuf (stripe#589) Build public docker images with 1.11.3 and alpine 3.8 (stripe#593) Document trace API (stripe#581) Add support for Datadog distribution metric type (stripe#590) Open CHANGELOG for v11.0.0 (stripe#592) Update CHANGELOG for v10.0.0 release (stripe#591)
Summary
This PR is an attempt to clean up most of my biggest frustrations with the trace API so far: Lack of documentation & clear examples on how to use it.
I wrote up a few sections of godoc on how to use this in a real service, made some tested example code that should clear up some of the confusion from real users.
Motivation
User asks from all over; my own confusion about which function to use to start a span.
Test plan
Wrote tests & examples!
Rollout/monitoring/revert plan
Just merge, it's only docs & tests.
R, @ChimeraCoder?
Cc @aubrey-stripe @clin-stripe - I'd love to know if this makes the package any more useful!