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.

Sign up for GitHub

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

Jump to bottom

fasthttptrace: Add trace context propagation support for libraries built on fasthttp #2218

Merged
merged 27 commits into from
Sep 28, 2023
Merged

fasthttptrace: Add trace context propagation support for libraries built on fasthttp #2218

merged 27 commits into from
Sep 28, 2023
+210 −14

Conversation

mtoffl01
Copy link
Contributor

@mtoffl01 mtoffl01 commented Sep 14, 2023 •
edited
Loading

What does this PR do?

  • Add foundational support for tracing libraries built on fasthttp, including a fasthttp implementation of TextMapCarrier and a fasthttp implementation of StartSpanFromContext
  • Move the contextKey type and ActiveSpanKey into ddtrace 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

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.

For Datadog employees:

  • If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@mtoffl01 mtoffl01 requested a review from a team September 14, 2023 19:08
@mtoffl01 mtoffl01 requested a review from a team as a code owner September 14, 2023 19:08
@pr-commenter
Copy link

pr-commenter bot commented Sep 14, 2023 •
edited
Loading

Benchmarks

Benchmark execution time: 2023-09-28 19:04:08

Comparing candidate commit 32d3325 in PR branch mtoff/fasthttp-support with baseline commit 04c819d in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 41 metrics, 0 unstable metrics.

rarguelloF
rarguelloF requested changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@rarguelloF rarguelloF left a 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

contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go Outdated Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go Outdated Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttptrace_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/context.go Outdated Show resolved Hide resolved
ddtrace/tracer/context_test.go Outdated Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go Outdated Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go Outdated Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go Outdated Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go Outdated Show resolved Hide resolved
mtoffl01 and others added 9 commits September 21, 2023 12:08
@mtoffl01 @rarguelloF
Update contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go
33cdf8a 
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
@mtoffl01 @rarguelloF
Update contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go
0c7bc8a 
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
@mtoffl01 @rarguelloF
Update contrib/internal/fasthttptrace/fasthttptrace_test.go
316a75d 
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
@mtoffl01 @rarguelloF
Update ddtrace/tracer/context.go
6566f89 
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
@mtoffl01 @rarguelloF
Update ddtrace/tracer/context_test.go
504a242 
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
@mtoffl01 @rarguelloF
Update contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go
a4b940f 
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
@mtoffl01 @rarguelloF
Update contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go
fcc0fd0 
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
@mtoffl01 @rarguelloF
Update contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go
7c621ae 
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
@mtoffl01 @rarguelloF
Update contrib/internal/fasthttptrace/fasthttpheaderscarrier_test.go
0e1183d 
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
github-actions[bot]
github-actions bot reviewed Sep 21, 2023
View reviewed changes
ddtrace/tracer/context.go Show resolved Hide resolved
ddtrace/tracer/context.go Outdated Show resolved Hide resolved
ddtrace/tracer/context.go Outdated Show resolved Hide resolved
ddtrace/tracer/context.go Show resolved Hide resolved
ddtrace/tracer/context.go Outdated Show resolved Hide resolved
ddtrace/tracer/context_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/context_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/context.go Show resolved Hide resolved
ddtrace/tracer/context.go Outdated Show resolved Hide resolved
ddtrace/tracer/context.go Outdated Show resolved Hide resolved
ahmed-mez
ahmed-mez reviewed Sep 22, 2023
View reviewed changes
Copy link
Contributor

@ahmed-mez ahmed-mez left a 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!

contrib/internal/fasthttptrace/fasthttpheaderscarrier.go Outdated Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttptrace.go Outdated Show resolved Hide resolved
contrib/google.golang.org/grpc/stats_client.go Outdated Show resolved Hide resolved
@mtoffl01 @ahmed-mez
Update contrib/internal/fasthttptrace/fasthttptrace.go
0ad6cc2 
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>
@mtoffl01 mtoffl01 changed the title Mtoff/fasthttp support fasthttptrace: Add trace context propagation support for libraries built on fasthttp Sep 26, 2023
ahmed-mez
ahmed-mez approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@ahmed-mez ahmed-mez left a 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!

contrib/internal/fasthttptrace/fasthttpheaderscarrier.go Outdated Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttpheaderscarrier.go Outdated Show resolved Hide resolved
mtoffl01 and others added 2 commits September 27, 2023 07:24
@mtoffl01 @ahmed-mez
Update contrib/internal/fasthttptrace/fasthttpheaderscarrier.go
21dd3a6 
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>
@mtoffl01 @ahmed-mez
Update contrib/internal/fasthttptrace/fasthttpheaderscarrier.go
b55e791 
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>
rarguelloF
rarguelloF approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@rarguelloF rarguelloF 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 just left some comments for missing renames for the StartSpanFromFastHTTPContext -> StartSpanFromContext refactor)

contrib/internal/fasthttptrace/fasthttptrace.go Outdated Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttpheaderscarrier.go Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttptrace.go Outdated Show resolved Hide resolved
contrib/internal/fasthttptrace/fasthttptrace_test.go Outdated Show resolved Hide resolved
@katiehockman katiehockman merged commit 93d4ab3 into main Sep 28, 2023
52 checks passed
@katiehockman katiehockman deleted the mtoff/fasthttp-support branch September 28, 2023 19:35
@mtoffl01 mtoffl01 mentioned this pull request Dec 4, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions left review comments

@rarguelloF rarguelloF rarguelloF approved these changes

@ahmed-mez ahmed-mez ahmed-mez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mtoffl01 @rarguelloF @ahmed-mez @katiehockman