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/fiber: upgrade to v2.50.0 and fix breaking change #2277

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

katiehockman
Copy link
Contributor

@katiehockman katiehockman commented Oct 17, 2023

What does this PR do?

  1. Upgrades gofiber/fiber to v2.50.0
  2. Fix the compilation issues after gofiber/fiber is upgraded

Unfortunately, I couldn't find a good way to unit test this. Mostly, I want to make sure that h.Add(k, v) in line 57 is used, since it allows for appending multiple headers with the same value, instead of h.Set(k, v), which would overwrite the values incorrectly. This shouldn't be all that essential though, since these headers are only used to extract the span context of the parent, which doesn't rely on headers that need multiple values (e.g. uses trace-id headers).
The only way I could see to test this would be to break out the creation of the http.Header{} type into a helper method, and doing a small unit test on the helper method. I haven't done that at this point, but I'd like reviewer feedback on whether there is a better way to test that, or if they feel that unit test would be helpful. But again, I'm not sure it's worth it.

Fixes #2275

Motivation

There was a critical security fix that was released in gofiber/fiber, which we should upgrade to. Unfortunately, the release with the security fix also included a breaking change to GetReqHeaders() by changing the return type. Our code needed to be changed to avoid compilation issues when users upgrade.

See https://github.com/gofiber/fiber/releases/tag/v2.50.0 for more details.

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!

@katiehockman katiehockman added apm:ecosystem contrib/* related feature requests or bugs tracer labels Oct 17, 2023
@katiehockman katiehockman requested a review from a team October 17, 2023 18:47
@katiehockman katiehockman requested a review from a team as a code owner October 17, 2023 18:47
@ajgajg1134
Copy link
Contributor

I think you're fine here from a testing perspective given there's already a TestPropagation unit test for gofiber here that ensure that propagation works (and I tested locally that if you comment out the Add call entirely the test will correctly fail.

@katiehockman katiehockman merged commit 75acbfa into main Oct 18, 2023
56 checks passed
@katiehockman katiehockman deleted the katie.hockman/gofiber-v2.50.0 branch October 18, 2023 12:41
@darccio darccio changed the title conbrib/gofiber/fiber: upgrade to v2.50.0 and fix breaking change contrib/gofiber/fiber: upgrade to v2.50.0 and fix breaking change Oct 19, 2023
darccio pushed a commit that referenced this pull request Oct 19, 2023
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 tracer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] gofiber/fiber v2.50.0 breaking changes
3 participants