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.

Sign up for GitHub

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

Jump to bottom

build: Update axum and http #2864

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

build: Update axum and http #2864

wants to merge 16 commits into from

Conversation

jan-auer
Copy link
Member

We would like to update the Sentry SDK, which requires to update axum and related dependencies. As reqwest has not updated to the latest version of http and hyper yet, we will temporarily run two versions of the frameworks side-by-side and convert between their types in the forward endpoint.

The axum-server library also has not released their latest version yet. Since the only thing needed from axum-server is graceful shutdown, we implement this ourselves now following the example in the axum repository.

#skip-changelog

@jan-auer jan-auer self-assigned this Dec 18, 2023
@jan-auer jan-auer requested a review from a team as a code owner December 18, 2023 13:38
@jan-auer
Copy link
Member Author

This PR caused an increase in CPU utilization, which needs to be fixed before merging this again.

@iker-barriocanal
Copy link
Member

iker-barriocanal commented Dec 19, 2023 •
edited

What's the benefit of this upgrade? (found some more context) I wonder because this may cause some confusion:

we will temporarily run two versions of the frameworks side-by-side and convert between their types in the forward endpoint.

@jan-auer
Merge branch 'master' into build/update-http-axum
b2a1a5c 
* master: (35 commits)
  fix(spans): Parse quotes in MySQL (#2846)
  ref(cardinality): Use a Lua script and in-memory cache for the cardinality limiter (#2849)
  fix(spans): Detect hex with fallback scrubber (#2868)
  release: 23.12.0
  Revert "ci: Update upload-artifact and download-artifact actions" (#2866)
  Revert "build: Update axum and http" (#2863)
  feat(spans): Allow resource.img spans (#2855)
  build: Update axum and http (#2844)
  fix(build): Add additional dependencies to the release build (#2858)
  ci: Update upload-artifact and download-artifact actions (#2861)
  feat(spans): Parse timestamps from strings (#2857)
  fix(spans): Scrub integer file extensions (#2856)
  feat(spans): Remove unused transaction tag from resource metrics (#2853)
  ref(cardinality): Recover buckets on cardinality limiter failure (#2852)
  feat(server): Org rate limit per metric bucket (#2836)
  ref(spans): List metric tags explicitly (#2834)
  feat(spans): Resource response sizes as measurements (#2845)
  feat(crons): Add thresholds to monitor config payload (#2842)
  feat(spans): Allow ingestion of metrics summary on spans (#2823)
  ref(crons): Add documentation to CheckInMessageType (#2840)
  ...
@jan-auer
build: Upgrade hyper to 1.1.0
ccc1b44
@jan-auer
ref(server): Re-introduce support for TCP keepalive
56477c5
olksdr
olksdr approved these changes Dec 19, 2023
View reviewed changes
Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

:shipit: let's try it again

@Dav1dde
Copy link
Member

Dav1dde commented Dec 19, 2023

:shipit: let's try it again

Do we have an idea what caused the spike?

@jan-auer
Copy link
Member Author

jan-auer commented Dec 20, 2023 •
edited

What's the benefit of this upgrade?

See the pull request description. We'd like to upgrade the Sentry SDK and since the axum integration there already depends on version 0.7, we'll have to do the update now. Alternatively, we could also vendor the integration and disable the feature flag, but there are some nice code improvements that come with the new Request type.

Do we have an idea what caused the spike?

Not yet sure. In the last attempt, we did not have TCP keepalive configured on the socket, as hyper has dropped that functionality. In 56477c5, I'm doing this manually now in the same way that hyper configured it.

@jan-auer
Copy link
Member Author

Another test showed that there's a single thread in main-rt that is running at 100% CPU utilization. We have not been able to pull a stack trace from that thread yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olksdr olksdr olksdr approved these changes

Assignees

@jan-auer jan-auer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jan-auer @iker-barriocanal @Dav1dde @olksdr