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

RFC: far fewer types (but comparable portability) #40

Merged
merged 11 commits into from
Jan 27, 2016
Merged

RFC: far fewer types (but comparable portability) #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7ba7985
compiles and runs (but grossness everywhere in standardtracer)
Jan 23, 2016
41f5970
one fewer interface since encode/decode weren't symmetric anyway
Jan 24, 2016
1c71173
better comments
Jan 24, 2016
347c361
more cleanups
Jan 24, 2016
56cca52
add SetOperationName(); remove an unneeded method;
Jan 25, 2016
cc02d3f
cleanups and file renames
Jan 25, 2016
eb53dfe
the ProcessIdentifier wasn't really needed; removed it.
Jan 25, 2016
27effd4
fix tests
Jan 25, 2016
048371a
rename param
Jan 25, 2016
b8b5353
resolve merge conflicts post-https://github.com/opentracing/opentraci…
Jan 26, 2016
ca5c92c
minor comment fix
Jan 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 18 additions & 34 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ demonstrating some important use cases.

The simplest starting point is `./default_tracer.go`. As early as possible, call

```
```go
import ".../opentracing-go"
import ".../some_tracing_impl"

Expand All @@ -37,7 +37,7 @@ If you prefer direct control to singletons, manage ownership of the

#### Starting an empty trace by creating a "root span"

```
```go
func xyz() {
...
sp := opentracing.StartTrace("span_name")
Expand All @@ -49,7 +49,7 @@ If you prefer direct control to singletons, manage ownership of the

#### Creating a Span given an existing Span

```
```go
func xyz(parentSpan opentracing.Span, ...) {
...
sp := opentracing.JoinTrace("span_name", parentSpan)
Expand All @@ -64,10 +64,11 @@ If you prefer direct control to singletons, manage ownership of the
Additionally, this example demonstrates how to get a `context.Context`
associated with any `opentracing.Span` instance.

```
```go
func xyz(goCtx context.Context, ...) {
...
sp, goCtx := opentracing.JoinTrace("span_name", goCtx).AddToGoContext(goCtx)
goCtx, sp := opentracing.ContextWithSpan(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, I was just tinkering with whether Span really needed that AddToGoContext helper method... the change here is not related to the main event in this PR.

goCtx, opentracing.JoinTrace("span_name", goCtx))
defer sp.Finish()
sp.LogEvent("xyz_called")
...
Expand All @@ -76,16 +77,16 @@ associated with any `opentracing.Span` instance.

#### Serializing to the wire

```
```go
func makeSomeRequest(ctx context.Context) ... {
if span := opentracing.SpanFromGoContext(ctx); span != nil {
if span := opentracing.SpanFromContext(ctx); span != nil {
httpClient := &http.Client{}
httpReq, _ := http.NewRequest("GET", "http://myservice/", nil)

// Transmit the span's TraceContext as an HTTP header on our
// outbound request.
opentracing.AddTraceContextToHeader(
span.TraceContext(),
opentracing.GlobalTracer().PropagateSpanInHeader(
span,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

important difference: we propagate Span instances, not TraceContexts.

httpReq.Header,
opentracing.DefaultTracer())

Expand All @@ -98,23 +99,16 @@ associated with any `opentracing.Span` instance.

#### Deserializing from the wire

```
```go
http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
// Grab the TraceContext from the HTTP header using the
// opentracing helper.
reqTraceCtx, err := opentracing.TraceContextFromHeader(
req.Header, opentracing.GlobalTracer())
var serverSpan opentracing.Span
var goCtx context.Context = ...
// Join the trace in the HTTP header using the opentracing helper.
serverSpan, err := opentracing.GlobalTracer().JoinTraceFromHeader(
"serverSpan", req.Header, opentracing.GlobalTracer())
if err != nil {
// Just make a root span.
serverSpan, goCtx = opentracing.StartTrace("serverSpan").AddToGoContext(goCtx)
} else {
// Make a new server-side span that's a child of the span/context sent
// over the wire.
serverSpan, goCtx = opentracing.JoinTrace(
"serverSpan", reqTraceCtx).AddToGoContext(goCtx)
serverSpan = opentracing.StartTrace("serverSpan")
}
var goCtx context.Context = ...
goCtx, _ = opentracing.ContextWithSpan(goCtx, serverSpan)
defer serverSpan.Finish()
...
}
Expand All @@ -127,14 +121,4 @@ synchronization.

## API pointers for those implementing a tracing system

There should be no need for most tracing system implementors to worry about the
`opentracing.Span` or `opentracing.Tracer` interfaces directly:
`standardtracer.New(...)` should work well enough in most circumstances.

In order to integrate with `standardtracer`, tracing system authors are
expected to provide implementations of:
- `opentracing.TraceContext`
- `opentracing.TraceContextSource`
- `standardtracer.Recorder`

For a small working example, see `../examples/dapperish/*.go`.
Tracing system implementors may be able to reuse or copy-paste-modify the `./standardtracer` package. In particular, see `standardtracer.New(...)`.
71 changes: 0 additions & 71 deletions codec.go
View file
Open in desktop

This file was deleted.

28 changes: 9 additions & 19 deletions examples/dapperish.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"bufio"
"bytes"
"fmt"
"html"
"io/ioutil"
"log"
"net/http"
Expand All @@ -19,10 +18,10 @@ import (
func client() {
reader := bufio.NewReader(os.Stdin)
for {
span := opentracing.StartTrace("getInput")
ctx := opentracing.BackgroundGoContextWithSpan(span)
ctx, span := opentracing.BackgroundContextWithSpan(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file gives a sense of how user code differs... mainly just that TraceContext is no longer there.

opentracing.StartTrace("getInput"))
// Make sure that global trace tag propagation works.
span.TraceContext().SetTraceAttribute("User", os.Getenv("USER"))
span.SetTraceAttribute("User", os.Getenv("USER"))
Copy link

Choose a reason for hiding this comment

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

don't mean to distract the issue, but I will 📦

I really like Tag Sets where propagation is simply a property of a tag. The "Trace" part of the name "TraceAttribute" has resulted in heaps of discussion, notably as no-one can guess by its name that it a propagation tag, but also the common "is it a part of the trace?" Ex. in zipkin trace id, id, parent id, and sampled decision are "trace attributes", ie only one field has anything to do with the trace.

I really would rather we be able to deconflate this whole thing by saying Span.setPropagationTag("User", os.Getenv("USER")), or worse but still better Span.setPropagationAttribute("User", os.Getenv("USER")). I have consistently found TraceAttribute an unnecessarily distracting term.

Copy link

Choose a reason for hiding this comment

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

Yes. "TraceAttribute" implies something that applies to the root, or to all parent Spans as well as child Spans.

Another suggestion for naming:

Span.setCascadedTag("User", os.Getenv("USER"))

It's an attribute that child tags will also inherit, regardless of network/process boundaries.

I find the whole "Distributed Context Propagation" at the API level, has grown into an unneeded (psychological) schema.

To the user of the API it can be simplified just as an attribute the casades, even over the network/process boundaries.

( The "Distributed Context Propagation" schema is still valuable in Specification though. )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I, too, am unnerved by the naming around "Trace Attributes"... if we pursue the basic model of this PR, something like SetCascadedTag is worth considering seriously (since both the cascaded and "non-cascaded" tags apply to the same object now: the Span).

@adriancole @michaelsembwever @yurishkuro thoughts on something like the following three methods on Span?

    SetTag(string, BasicType)
    SetCascadingTag(string, BasicType)
    Tag(string) --> string

Some problems right off the bat:

  • If "cascading tags" (trace attributes) are to be used as HTTP headers without bizarro escaping, we run headlong into the lengthy/complex caveats about casing, hyphens, etc. And it seems like the key restrictions for "plain" tags and cascading tags should be identical.
  • What to do if there's a "cascading" tag and a user calls SetTag with the same key? Who wins?

Copy link

Choose a reason for hiding this comment

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

👍 -- Attribute vs Tag was a strange terminology distinction when I first started getting into this project; nothing unusual but the kind of naming that software is unfortunately rife with. So I think it can be clearer.

However, I'm not sure folding Attributes into Span Tags (via CascadingTag or similar) is going to solve the problem. A Tag is data added to a span, whereas an Attribute is data added to a Context. Based on that Attributes feel like they are not part of Span creation. We do want to propagate them, but they are not Span-centric.

It makes sense to think of the relevant part of DCP as propagating a Span when you have a zipkin-like model where client and server report data into the same Span, but when considering more generalized tracing systems, Context is the thing that is propagated, and Spans are created to report information on execution as the Context passes through. We can add Span info to the Context before propagating to inform the structure of the tree and Spans, but that doesn't make the Context a Span.

I realize this is arguably scope question currently being debated in #33 as well, but clarity is the goal of this line of discussion and I think it will be clearer if we don't conflate these two concepts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(/me files a motion to decouple this important, unfinished discussion from the current PR)

span.LogEventWithPayload("ctx", ctx)
fmt.Print("\n\nEnter text (empty string to exit): ")
text, _ := reader.ReadString('\n')
Expand All @@ -36,8 +35,8 @@ func client() {

httpClient := &http.Client{}
httpReq, _ := http.NewRequest("POST", "http://localhost:8080/", bytes.NewReader([]byte(text)))
opentracing.AddTraceContextToHeader(
span.TraceContext(), httpReq.Header, opentracing.GlobalTracer())
opentracing.PropagateSpanInHeader(
Copy link

Choose a reason for hiding this comment

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

Existing tracers send via multiple headers, but I guess this is named PropagateSpanInHeader vs PropagateSpanInHeaders bc in go, headers are in an object named Header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I don't like it, but that's what the Go maintainers decided, apparently.

span, httpReq.Header, opentracing.GlobalTracer())
resp, err := httpClient.Do(httpReq)
if err != nil {
span.LogEventWithPayload("error", err)
Expand All @@ -51,28 +50,19 @@ func client() {

func server() {
http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) {
reqCtx, err := opentracing.TraceContextFromHeader(
req.Header, opentracing.GlobalTracer())
serverSpan, err := opentracing.JoinTraceFromHeader(
"serverSpan", req.Header, opentracing.GlobalTracer())
if err != nil {
panic(err)
}

serverSpan := opentracing.JoinTrace(
"serverSpan", reqCtx,
).SetTag("component", "server")
serverSpan.SetTag("component", "server")
defer serverSpan.Finish()

fullBody, err := ioutil.ReadAll(req.Body)
if err != nil {
serverSpan.LogEventWithPayload("body read error", err)
}
serverSpan.LogEventWithPayload("got request with body", string(fullBody))
contextIDMap, tagsMap := opentracing.TraceContextToText(reqCtx)
fmt.Fprintf(
w,
"Hello: %v // %v // %q",
contextIDMap,
tagsMap,
html.EscapeString(req.URL.Path))
})

log.Fatal(http.ListenAndServe(":8080", nil))
Expand Down
Loading