Skip to content
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

Trace propagation support #557

Closed
wants to merge 1 commit into from
Closed

Trace propagation support #557

wants to merge 1 commit into from

Conversation

viveklak
Copy link
Contributor

@viveklak viveklak commented Jul 1, 2022

No description provided.

@viveklak viveklak requested a review from lblackstone July 1, 2022 22:24
@viveklak viveklak added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Jul 1, 2022
@viveklak viveklak requested a review from stack72 July 1, 2022 22:24
@github-actions
Copy link

github-actions bot commented Jul 1, 2022

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link
Member

@lblackstone lblackstone left a comment

Choose a reason for hiding this comment

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

Couple of small comments, but LGTM overall

Comment on lines +126 to +127
// ctx, cancelFunc := context.WithTimeout(ctx, timeout)
// defer cancelFunc()
Copy link
Member

Choose a reason for hiding this comment

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

Still needed?

name := fmt.Sprintf("%s@%s", req.Method, req.URL.Path)
span, ctx := opentracing.StartSpanFromContext(ctx, name)
defer span.Finish()
req2 := req.WithContext(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest a more descriptive name for req2. Maybe reqWithContext?

@mjeffryes
Copy link
Contributor

closing as stale

@mjeffryes mjeffryes closed this Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants