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

Reduce allocation cost reporting spans #35183

Merged
merged 1 commit into from
Aug 4, 2023
Merged

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Aug 3, 2023

This is done by leveraging Vert.x's Buffer
directly instead of relying on a
ByteArrayOutputStream

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 3, 2023

/cc @brunobat (opentelemetry), @radcortez (opentelemetry)

@brunobat
Copy link
Contributor

brunobat commented Aug 3, 2023

Just to better understand this change... Will the Buffer be reused inside vert.x thus reducing the allocation needs?
The Javadoc was not clear enough.

@quarkus-bot

This comment has been minimized.

@brunobat
Copy link
Contributor

brunobat commented Aug 3, 2023

btw, main is broken. Failure in dev-ui-resources is unrelated.

@geoand
Copy link
Contributor Author

geoand commented Aug 3, 2023

Will the Buffer be reused inside vert.x thus reducing the allocation needs?
The Javadoc was not clear enough.

Inside Vert.x / Netty, it will.

However currently, the buffer is not pooled, meaning that each reporting of Spans will result in the allocation of a new memory segment.
If we decide to forgo allocations completely in this piece of code (cc @franz1981 @cescoffier), then this code makes it trivial, as all we have to do use the proper allocator (and guard against any exception)

@geoand
Copy link
Contributor Author

geoand commented Aug 3, 2023

btw, main is broken. Failure is unrelated.

Yup, unfortunately

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

This is done by leveraging Vert.x's Buffer
directly instead of relying on a
ByteArrayOutputStream
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 3, 2023

Failing Jobs - Building d603488

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 20

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: extensions/vertx-http/deployment 
! Skipped: extensions/agroal/deployment extensions/amazon-lambda-http/deployment extensions/amazon-lambda-rest/deployment and 358 more

📦 extensions/vertx-http/deployment

Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.1.2:test (default-test) on project quarkus-vertx-http-deployment: There was a timeout in the fork

@geoand geoand merged commit 0d3cb35 into quarkusio:main Aug 4, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Aug 4, 2023
@geoand geoand deleted the spans-alloc branch August 4, 2023 16:31
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