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

Add export sync #5105

Merged
merged 7 commits into from
Mar 27, 2024
Merged

Add export sync #5105

merged 7 commits into from
Mar 27, 2024

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Mar 25, 2024

The batching log processor will generate records from 4 different locations (polling, OnEmit, ForceFlush, Shutdown). In order to ensure an Exporter is called serially, as is required by the interface, this function will be used in the processor to centralize the interaction with its Exporter.

Part of #5063.

See #5093 for the implementation use.

The batching log processor will generate records from 4 different
locations (polling, OnEmit, ForceFlush, Shutdown). In order to ensure an
Exporter is called serially, as is required by the interface, this
function will be used in the processor to centralize the interaction
with its Exporter.

Part of open-telemetry#5063.

See open-telemetry#5093 for the implementation use.
@MrAlias MrAlias added area:logs Part of OpenTelemetry logs Skip Changelog PRs that do not require a CHANGELOG.md entry labels Mar 25, 2024
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.8%. Comparing base (068b6c8) to head (b1c9851).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #5105   +/-   ##
=====================================
  Coverage   83.8%   83.8%           
=====================================
  Files        248     248           
  Lines      16203   16221   +18     
=====================================
+ Hits       13581   13599   +18     
  Misses      2334    2334           
  Partials     288     288           
Files Coverage Δ
sdk/log/exporter.go 100.0% <100.0%> (ø)

Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Just curious, why did you choose this approach, rather than just locking around the exporter?

Copy link
Member

@XSAM XSAM left a comment

Choose a reason for hiding this comment

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

👍

@MrAlias
Copy link
Contributor Author

MrAlias commented Mar 27, 2024

Just curious, why did you choose this approach, rather than just locking around the exporter?

Locking around the exporter would mean the calling goroutine would need to wait for the export to complete before continuing. Either the batching processor routine, or the polling routine will be blocked in that case. Either of which is going to be problematic as it could cause large back-pressure for systems with high throughput.

Passing via a channel to a separate goroutine will allow the the export to not block, and if the export becomes excessively slow we can start dropping payloads instead of locking up.

sdk/log/exporter.go Show resolved Hide resolved
@MrAlias MrAlias merged commit 321219b into open-telemetry:main Mar 27, 2024
27 checks passed
@MrAlias MrAlias deleted the add-export-sync branch March 27, 2024 18:45
@MrAlias MrAlias added this to the v1.25.0 milestone Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logs Part of OpenTelemetry logs Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants