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] Fix crash in ElasticsearchLogRecordExporter #3082

Merged

Conversation

ShadowMaxLeb
Copy link
Contributor

@ShadowMaxLeb ShadowMaxLeb commented Oct 7, 2024

Fixes #3078

Using constructor with user provided options in ElasticsearchLogRecordExporter was making the program crash due to synchronization_data_ not being intialized when built with async behavior (-DWITH_ASYNC_EXPORT_PREVIEW=ON)

Changes

Constructors have been changed so the default one always calls the one with options as parameters to ensure to always initialize everything.

@ShadowMaxLeb ShadowMaxLeb requested a review from a team as a code owner October 7, 2024 10:10
Copy link

linux-foundation-easycla bot commented Oct 7, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Oct 7, 2024

Deploy Preview for opentelemetry-cpp-api-docs canceled.

Name Link
🔨 Latest commit d010ced
🔍 Latest deploy log https://app.netlify.com/sites/opentelemetry-cpp-api-docs/deploys/6704c13624708500081783ea

@marcalff marcalff added the pr:waiting-on-cla Waiting on CLA label Oct 7, 2024
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.82%. Comparing base (497eaf4) to head (d010ced).
Report is 137 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3082      +/-   ##
==========================================
+ Coverage   87.12%   87.82%   +0.71%     
==========================================
  Files         200      195       -5     
  Lines        6109     5968     -141     
==========================================
- Hits         5322     5241      -81     
+ Misses        787      727      -60     

see 125 files with indirect coverage changes

@marcalff
Copy link
Member

marcalff commented Oct 7, 2024

Thanks @ShadowMaxLeb for the patch.

Please sign the contributor agreement (CLA), so this PR can be merged to the main branch.
The CLA will apply to all PR, present and future, so it only needs to be done once.

@marcalff marcalff removed the pr:waiting-on-cla Waiting on CLA label Oct 7, 2024
Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix.

@marcalff
Copy link
Member

marcalff commented Oct 7, 2024

@ShadowMaxLeb

Thanks for the contribution.

This PR is approved and ready to be merged to the main branch.
Unfortunately, we (opentelemetry-cpp maintainers) do not have github permissions to update this branch.
This is needed to:

  • perform a main -> PR merge first, to align the PR branch to main
  • perform the PR -> main merge, to accept the contribution.

Please allow maintainers to make edits (there is a checkbox in the github UI for that), or perform a main -> PR merge so we can take it.

Thanks.

@marcalff marcalff added the pr:fix-merge-conflicts Please fix merge conflicts for this pr label Oct 7, 2024
@marcalff
Copy link
Member

marcalff commented Oct 7, 2024

Added the merge conflict tag.
There are no merge conflicts, only the down merge needs to be resolved:

This branch is out-of-date with the base branch

@ShadowMaxLeb
Copy link
Contributor Author

Since the fork ispart of an organization, it seems that I do not have the possibility to allow maintainers (https://github.com/orgs/community/discussions/5634).

I will update the PR.

@marcalff marcalff removed the pr:fix-merge-conflicts Please fix merge conflicts for this pr label Oct 8, 2024
@marcalff marcalff merged commit 0ea1f2c into open-telemetry:main Oct 8, 2024
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ElasticSearch exporter crashes when emitting a log (EXC_BAD_ACCESS)
3 participants