Skip to content

Conversation

@kevpar
Copy link
Member

@kevpar kevpar commented Aug 24, 2019

No description provided.

@kevpar kevpar requested a review from a team August 24, 2019 19:11
Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
kevpar added 2 commits August 24, 2019 12:21
Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
Signed-off-by: Kevin Parsons <kevpar@microsoft.com>
Copy link
Contributor

@jterry75 jterry75 left a comment

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
Member Author

kevpar commented Aug 25, 2019

@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 kevpar merged commit d64a16f into master Aug 25, 2019
@jterry75
Copy link
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.

@jterry75 jterry75 deleted the octtrpc branch August 25, 2019 17:29
@kiashok kiashok mentioned this pull request Dec 6, 2024
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