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

contrib/valyala/fasthttp.v1: add a fasthttp integration to ddtrace #2305

Merged
merged 19 commits into from Dec 18, 2023

Conversation

mtoffl01
Copy link
Contributor

@mtoffl01 mtoffl01 commented Oct 27, 2023

What does this PR do?

Provides middleware to trace requests handled with the FastHTTP library: https://pkg.go.dev/github.com/valyala/fasthttp

Motivation

Customer requests!
#700

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 October 27, 2023 20:46
@github-actions github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Oct 27, 2023
@pr-commenter
Copy link

pr-commenter bot commented Oct 27, 2023

Benchmarks

Benchmark execution time: 2023-12-18 10:24:25

Comparing candidate commit a083689 in PR branch mtoff/fasthttp with baseline commit e44bd72 in branch main.

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

@mtoffl01
Copy link
Contributor Author

mtoffl01 commented Oct 27, 2023

The unit tests fail when run with -shuffle=on. They pass with -race and -parallel=N. For some reason, when they are run with shuffle, each test has an additional client net/http span, which I can't understand because the only test where I instrument the client call is TestPropagation.

Also, tests pass with the -shuffle=on flag if I comment out either TestPropagation or TestChildSpan 🤔
Can reproduce this issue running go test -shuffle=on inside of dd-trace-go/contrib/valyala/fasthttp.v1.

This is resolved in commit #43a2ed7.

@felixge
Copy link
Member

felixge commented Oct 31, 2023

The unit tests fail when run with -shuffle=on

Is this when running the tests for the new package you're adding, or the whole test suite? The latter doesn't work with -shuffle=on yet AFAIK, but it'd be great for new packages to pass with this flag enabled.

Let me know if you need help debugging.

@mtoffl01
Copy link
Contributor Author

mtoffl01 commented Nov 1, 2023

I want to make sure we add documentation for how to use the integration, like the example for net/http contrib - https://pkg.go.dev/gopkg.in/DataDog/dd-trace-go.v1/contrib/net/http#example-package

Copy link

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale Stuck for more than 1 month label Nov 22, 2023
@github-actions github-actions bot removed the stale Stuck for more than 1 month label Nov 23, 2023
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.

Nice work! 🎉

Left some refactor suggestions. Also we should make sure we add an example_test.go file with a ExampleWrapHandler function to make sure we provide documentation to users on how to use the integration.

contrib/valyala/fasthttp.v1/fasthttp_test.go Outdated Show resolved Hide resolved
contrib/valyala/fasthttp.v1/fasthttp_test.go Outdated Show resolved Hide resolved
mtoffl01 and others added 2 commits November 30, 2023 16:59
Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com>
@darccio
Copy link
Contributor

darccio commented Dec 18, 2023

/merge

@dd-devflow
Copy link

dd-devflow bot commented Dec 18, 2023

🚂 MergeQueue

Pull request added to the queue.

This build is going to start soon! (estimated merge in less than 0s)

you can cancel this operation by commenting your pull request with /merge -c!

@dd-devflow
Copy link

dd-devflow bot commented Dec 18, 2023

🚨 MergeQueue

mergequeue build completed successfully, but the github api returned an error while merging the pr

Details

Error: PUT https://api.github.com/repos/DataDog/dd-trace-go/pulls/2305/merge: 405 5 of 5 required status checks are expected. []

FullStacktrace:
activity error (type: github.GithubService_MergePullRequest, scheduledEventID: 41, startedEventID: 42, identity: 1@github-worker-fb48fd8b7-zrxht@): PUT https://api.github.com/repos/DataDog/dd-trace-go/pulls/2305/merge: 405 5 of 5 required status checks are expected. [] (type: ErrorResponse, retryable: true)

If you need support, contact us on slack #ci-interfaces with those details!

@darccio darccio enabled auto-merge (squash) December 18, 2023 14:55
@darccio darccio merged commit ff5a05e into main Dec 18, 2023
153 checks passed
@darccio darccio deleted the mtoff/fasthttp branch December 18, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs mergequeue-status: error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants