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

[exporter/prometheusremotewrite] fix snappy double alloc #34274

Conversation

ben-childs-docusign
Copy link
Contributor

@ben-childs-docusign ben-childs-docusign commented Jul 26, 2024

Description:

Snappy Encode function is not using the buffer passed in by prometheus remote write as it is too small. Removing this unused buffer allocation and just passing nil. Snappy will allocate the required buffers.

Link to tracking Issue:
Fixes #34273

Testing:
Ran existing unit tests which cover this code

Ran this in our integration environment and verified reduced allocations.
Before:
dfce1b99-8998-422e-a9e6-ccc5f6401393

After:
image

Documentation:
Updated changelog

@rapphil
Copy link
Contributor

rapphil commented Jul 29, 2024

There are a few approval steps that are failing, can you take a look to make sure that they are not related to this change or require merging from mainline?

Otherwise this change looks good to me.

Co-authored-by: Matthew Wear <matthew.wear@gmail.com>
@ben-childs-docusign
Copy link
Contributor Author

@mwear , @rapphil thanks for the review - whats the next step to get this merged? I checked the latest build failure and it seems to be a transformprocessor unit test which isn't impacted by this change afaict.

@rapphil
Copy link
Contributor

rapphil commented Jul 30, 2024

@ben-childs-docusign , It is possible that it is a flaky test. can you take a look into other PRs that were created recently and check if the tests are failing there as well? Is your branch updated with regards to main? As next steps, I think @mwear can add the ready to merge label and then some maintainer will pick it for merging.

@dmitryax dmitryax merged commit b418e59 into open-telemetry:main Jul 30, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 30, 2024
@ben-childs-docusign ben-childs-docusign deleted the bc--optimizesnappybufferallocation branch July 30, 2024 22:02
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.

[exporter/prometheusremotewrite] Double allocation in execute function due to snappy buffer size mismatch
5 participants