-
Notifications
You must be signed in to change notification settings - Fork 457
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
fasthttptrace: Add trace context propagation support for libraries built on fasthttp #2218
Conversation
…ted carrier and span generation methods for fasthttp
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 overall! left a few nit comments
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
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 believe this is the right approach, thanks for working on it!
Added a few minor suggestions but overall the code looks right, let's also rename the PR!
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>
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, feel free to merge without a second approval from me once the linter and remaining comments/issues are resolved. @rarguelloF please take a look as well. Thanks!
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>
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! (I just left some comments for missing renames for the StartSpanFromFastHTTPContext -> StartSpanFromContext refactor)
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
What does this PR do?
internal
package and make ActiveSpanKey accessible to other packages that may need it (such as fasthttp contrib packages)Motivation
In adding a contrib for gearbox (PR: #2118), we found that the existing functionality in ddtrace relied heavily on a net/http implementation. We needed new functionality to update the fasthttp request context and to propagate trace context via fasthttp request headers.
Reviewer's Checklist
For Datadog employees:
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!