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/gofiber: add gofiber trace propagation #1487

Merged
merged 12 commits into from
Nov 21, 2022

Conversation

mckeown-dd
Copy link
Contributor

This updates gofiber to the earliest version that gave a map for go fiber's request headers. It iterates through those, creates a http.Header object so that the tracer extraction process doesn't have to be modified.

@mckeown-dd mckeown-dd requested a review from a team September 29, 2022 01:30
@mckeown-dd mckeown-dd requested a review from a team as a code owner September 29, 2022 01:30
@mckeown-dd mckeown-dd added the enhancement quick change/addition that does not need full team approval label Sep 29, 2022
@mckeown-dd mckeown-dd added this to the Triage milestone Sep 29, 2022
@tylermichael
Copy link

I'm running into an issue where APM doesn't connect services because fiber doesn't read the request headers for the distributed span ID. This PR would fix that.

I think this also would have partially addressed this issue: #1222

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.

Hi @mckeown-dd, thanks for the contribution, can you add a unit test?

@ahmed-mez ahmed-mez linked an issue Nov 16, 2022 that may be closed by this pull request
@mckeown-dd
Copy link
Contributor Author

mckeown-dd commented Nov 18, 2022

@ahmed-mez. Can you help me figure out why the lint test is failing? Looks like there was already a unit test for propagation, but it didn't really catch the issue. I modified it to check that the trace id is the same as the parent if one is passed.

@ajgajg1134
Copy link
Contributor

@ahmed-mez. Can you help me figure out why the lint test is failing? Looks like there was already a unit test for propagation, but it didn't really catch the issue. I modified it to check that the trace id is the same as the parent if one is passed.

It's your imports list, we enforce a style guide on the grouping and ordering of them as defined here: https://github.com/DataDog/dd-trace-go/wiki/Style-guidelines#imports

It should be a nice quick and easy fix :)

@dianashevchenko dianashevchenko requested a review from a team as a code owner November 21, 2022 13:46
@pr-commenter

This comment has been minimized.

@DataDog DataDog deleted a comment from pr-commenter bot Nov 21, 2022
@pr-commenter
Copy link

pr-commenter bot commented Nov 21, 2022

Benchmarks

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

@ahmed-mez ahmed-mez removed this from the Triage milestone Nov 21, 2022
@ahmed-mez ahmed-mez added this to the v1.44.0 milestone Nov 21, 2022
@dianashevchenko dianashevchenko merged commit 2d50f9c into main Nov 21, 2022
@dianashevchenko dianashevchenko deleted the feature/gofiber-trace-propagation branch November 21, 2022 15:38
@dianashevchenko dianashevchenko changed the title add gofiber trace propagation contrib/gofiber: add gofiber trace propagation Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement quick change/addition that does not need full team approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] dd-trace-go does not propagate traces in fiber.
6 participants