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

opentelemetry_req: Don't assume request.headers shape #193

Merged
merged 2 commits into from
Oct 16, 2023

Conversation

wojtekmach
Copy link
Contributor

I'm planning to switch Req headers to be maps. This patch will work on either representation. After we switch to maps, Req.Request.put_header/3 should be plenty fast, however I'd be happy to send another patch that updates the headers in bulk if that'd be more desirable.

cc @bryannaegele

@wojtekmach
Copy link
Contributor Author

I just noticed there's a couple ongoing PRs:

I'm happy to wait until they are addressed and rebase on top. :)

Just FYI, the :headers change would only happen in Req v0.4.0 which is planned to happen in the next couple of weeks. Ideally :opentelemetry_req would have depended on {:req, "~> 0.3.0"}, that is, projects using both dependencies would not have picked up req v0.4.0 when it comes out. But I think it's gonna be ok, if we can get a new opentelemetry_req version with a patch like this one, I'm happy to mention the need to upgrade in my Req v0.4 changelog. Sorry for the extra hassle, if there's anything I can do to smooth the transition on req or opentelemtry_req, please let me know!

Copy link
Collaborator

@bryannaegele bryannaegele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wojtekmach and @andrewhr I'm going to go with this PR with the tests as-is in here so we can get a release out to support >= 0.4

@bryannaegele bryannaegele merged commit e1f4a02 into open-telemetry:main Oct 16, 2023
33 checks passed
@bryannaegele bryannaegele added the minor Minor version label Oct 16, 2023
@wojtekmach wojtekmach deleted the wm-req-headers branch October 17, 2023 16:49
@yordis yordis mentioned this pull request Dec 12, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants