Skip to content

Support forwarding OpenCensus spans over TTRPC for shim task service - #678

Merged
Kevin Parsons (kevpar) merged 3 commits into
masterfrom
octtrpc
Aug 25, 2019
Merged

Support forwarding OpenCensus spans over TTRPC for shim task service#678
Kevin Parsons (kevpar) merged 3 commits into
masterfrom
octtrpc

Conversation

@kevpar

Copy link
Copy Markdown
Member

No description provided.

@kevpar
Kevin Parsons (kevpar) requested a review from a team August 24, 2019 19:11
Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
Signed-off-by: Kevin Parsons <kevpar@microsoft.com>

@jterry75 Justin (jterry75) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. I'm only wondering about duplicate error logs but its not a huge deal

@kevpar

Copy link
Copy Markdown
Member Author

Justin (@jterry75) Assuming you mean duplicate error spans between the ones created by octtrpc and the "implementation side" span, my stance is we should adapt our queries to work with it. I think mostly from a query perspective we should be looking at errors in "user facing" spans (e.g. CRI entry points), so this shouldn't be an issue there. For the case of looking to see what are the top errors across all spans, we will have to deal with the duplicate, but I don't think it will be a big deal.

The behavior here also matches what is done with ocgrpc, so I think we'll need to deal with it even if we tried to do something else in octtrpc.

I'm going to check this in, but happy to discuss this further next week.

@kevpar
Kevin Parsons (kevpar) merged commit d64a16f into master Aug 25, 2019
@jterry75

Copy link
Copy Markdown
Contributor

Here is what I am wondering in a nutshell:

Here is what we do today:

func (s *svc) SomeMethod(ctx context.Context, p *Parameters) (r *Return, err error) {
    ctx, span := trace.StartSpan(ctx, "SomeMethod)
    span.AddAttributes(...) // stuff with `p`
    defer func() {
        if err == nil {
            span.AddAttributes(...) // stuf with `r`
        }
    }()
    // Method impl
}

But since we now create the entry spans automatically I was wondering if an entry method should now do:

func (s *svc) SomeMethod(ctx context.Context, p *Parameters) (r *Return, err error) {
    span := trace.FromContext(ctx)
    if span != nil {
        span.AddAttributes(...) // stuff with `p`
        defer func() {
            if err == nil {
                span.AddAttributes(...) // stuff with `r`
            }
        }()
    }
    // Method impl
}

So instead of overwriting the entry span with an immediate child we instead enlighten the attributes of an entry span.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants