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

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

Merged
merged 27 commits into from
Sep 28, 2023

Conversation

mtoffl01
Copy link
Contributor

@mtoffl01 mtoffl01 commented Sep 14, 2023

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!

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler
…ted carrier and span generation methods for fasthttp

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler
@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

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler
@pr-commenter
Copy link

pr-commenter bot commented Sep 14, 2023

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.

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

mtoffl01 and others added 9 commits September 21, 2023 12:08

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler
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!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler
…r than append

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler
@mtoffl01 mtoffl01 changed the title Mtoff/fasthttp support fasthttptrace: Add trace context propagation support for libraries built on fasthttp Sep 26, 2023
@mtoffl01 mtoffl01 requested a review from rarguelloF September 26, 2023 18:14

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler
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!

mtoffl01 and others added 2 commits September 27, 2023 07:24

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Ahmed Mezghani <38987709+ahmed-mez@users.noreply.github.com>

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler
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)

mtoffl01 and others added 4 commits September 28, 2023 10:10

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>

Verified

This commit was signed with the committer’s verified signature.
mtoffl01 Mikayla Toffler

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@katiehockman katiehockman merged commit 93d4ab3 into main Sep 28, 2023
@katiehockman katiehockman deleted the mtoff/fasthttp-support branch September 28, 2023 19:35
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.

None yet

4 participants