[SDK] Add `ForceFlush` for all `LogRecordExporter`s and `SpanExporter`s. #2000

owent · 2023-02-23T12:30:59Z

Fixes #1623
Fixes #1955

Changes

Add ForceFlush for all LogRecordExporters and SpanExporters.
Call ForceFlush to prevent cancel in OTLP examples.
Optimize BatchSpanProcessor::ForceFlush and BatchLogRecordProcessor::ForceFlush. We will finish force flush more quickly when pass a large timeout .
Optimize OTLP HTTP log example

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2023-02-23T12:58:34Z

Codecov Report

Merging #2000 (8359014) into main (3a09d53) will decrease coverage by 0.12%.
The diff coverage is 68.58%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2000      +/-   ##
==========================================
- Coverage   87.30%   87.17%   -0.12%     
==========================================
  Files         166      166              
  Lines        4723     4746      +23     
==========================================
+ Hits         4123     4137      +14     
- Misses        600      609       +9

Impacted Files	Coverage Δ
...de/opentelemetry/exporters/ostream/span_exporter.h	`100.00% <ø> (ø)`
exporters/ostream/src/span_exporter.cc	`88.12% <0.00%> (-2.69%)`	⬇️
...k/include/opentelemetry/sdk/trace/tracer_context.h	`100.00% <ø> (ø)`
sdk/src/trace/tracer.cc	`78.73% <0.00%> (-11.52%)`	⬇️
sdk/src/trace/exporter.cc	`50.00% <50.00%> (ø)`
...include/opentelemetry/sdk/trace/simple_processor.h	`100.00% <100.00%> (ø)`
sdk/src/trace/batch_span_processor.cc	`92.71% <100.00%> (+1.23%)`	⬆️
sdk/src/trace/tracer_context.cc	`81.82% <100.00%> (ø)`

... and 1 file with indirect coverage changes

sdk/include/opentelemetry/sdk/logs/exporter.h

examples/otlp/grpc_log_main.cc

lalitb

LGTM. With nit comments. Thanks.

CHANGELOG.md

exporters/etw/CMakeLists.txt

sdk/src/trace/batch_span_processor.cc

exporters/memory/include/opentelemetry/exporters/memory/in_memory_span_exporter.h

lalitb · 2023-03-17T07:36:43Z

@owent - Can you resolve conflict here too, we can merge it now.

lalitb · 2023-03-17T21:15:48Z

sdk/src/trace/batch_span_processor.cc

- break_condition);
- }
- result = true;
+ timeout_steady = std::chrono::steady_clock::duration::max();


I am not able to understand this part of code. The AdjustWaitForTimeout call above will change timeout from duration::zero() to duration::max() and this line now will change it back to duration::zero().

AdjustWaitForTimeout just cahnge the timeout to meet the requirement of wait_for .It will return the second parameter when the timeout is greater than max available value.It will change timeout to duration::zero() here, not to duration::max().
Some old codes use zero for indefinite waiting. Maybe we can raise another PR to remove this implementation if we have cleanup all zero value usage. Maybe it is easier to understand to do not wait when we get zero here.
What's your suggestion?

Yes, I think cleaning up the zero-value usage for indefinite waiting in a separate PR would be good. Thanks for the explanation.

+ Optimize `BatchSpanProcessor::ForceFlush` and `BatchLogRecordProcessor::ForceFlush`. + Optimize OTLP HTTP log example + Call `ForceFlush` to prevent cancel in OTLP examples. Signed-off-by: owent <[email protected]>

…nd `LogRecordExporter::ForceFlush`. Signed-off-by: owent <[email protected]>

…ter` into .cc files. Signed-off-by: owent <[email protected]>

Signed-off-by: owent <[email protected]>

…elemetry_logs` Signed-off-by: owent <[email protected]>

…ix the unit test in open-telemetry#1793 . Signed-off-by: owent <[email protected]>

Signed-off-by: owent <[email protected]>

owent · 2023-03-20T02:54:02Z

@owent - Can you resolve conflict here too, we can merge it now.

Done.

* commit '7887d32da60f54984a597abccbb0c883f3a51649': (82 commits) [RELEASE] Release version 1.9.0 (open-telemetry#2091) Use sdk_start_ts for MetricData start_ts for instruments having cumulative aggregation temporality. (open-telemetry#2086) [SEMANTIC CONVENTIONS] Upgrade to version 1.20.0 (open-telemetry#2088) [EXPORTER] Add OTLP HTTP SSL support (open-telemetry#1793) Make Windows build environment parallel (open-telemetry#2080) make some hints (open-telemetry#2078) Make some targets parallel in CI pipeline (open-telemetry#2076) [Metrics SDK] Implement Forceflush for Periodic Metric Reader (open-telemetry#2064) Upgraded semantic conventions to 1.19.0 (open-telemetry#2017) Bump actions/stale from 7 to 8 (open-telemetry#2070) Include directory path added for Zipkin exporter example (open-telemetry#2069) Ignore more warning of generated protobuf files than not included in `-Wall` and `-Wextra` (open-telemetry#2067) Add `ForceFlush` for all `LogRecordExporter`s and `SpanExporter`s. (open-telemetry#2000) Remove unused 'alerting' section from prometheus.yml in examples (open-telemetry#2055) Clean warnings in ETW exporters (open-telemetry#2063) Fix default value of `OPENTELEMETRY_INSTALL_default`. (open-telemetry#2062) [EXPORTER] GRPC endpoint scheme should take precedence over OTEL_EXPORTER_OTLP_TRACES_INSECURE (open-telemetry#2060) Fix view names in Prometheus example (open-telemetry#2034) Fix some docs typo (open-telemetry#2057) Checking indices before dereference (open-telemetry#2040) ... # Conflicts: # exporters/ostream/CMakeLists.txt # sdk/src/metrics/state/metric_collector.cc # sdk/src/metrics/state/temporal_metric_storage.cc

owent requested a review from a team February 23, 2023 12:31

owent changed the title ~~[WIP] Add ForceFlush for all LogRecordExporters and SpanExporters.~~ Add ForceFlush for all LogRecordExporters and SpanExporters. Feb 23, 2023

esigo added the size/XL Denotes a PR that changes 500-999 lines. label Feb 26, 2023

lalitb mentioned this pull request Feb 27, 2023

Prepare release v.1.9.0 (tentative: 3rd March) #2007

Closed

lalitb added this to the OpenTelemetry C++ Release v1.9.2 (Feb Release) milestone Feb 27, 2023

lalitb reviewed Mar 2, 2023

View reviewed changes

sdk/include/opentelemetry/sdk/logs/exporter.h Outdated Show resolved Hide resolved

lalitb reviewed Mar 2, 2023

View reviewed changes

examples/otlp/grpc_log_main.cc Outdated Show resolved Hide resolved

owent force-pushed the add_SpanExporter_ForceFlush branch 3 times, most recently from cd4a83f to 5e20900 Compare March 6, 2023 01:54

owent mentioned this pull request Mar 6, 2023

Prepare release, include fixes with abi breaking changes #1983

Closed

ThomsonTan mentioned this pull request Mar 6, 2023

Prepare v1.8.3 release #2018

Merged

3 tasks

lalitb modified the milestones: OpenTelemetry C++ Feb 2023 release , OpenTelemetry C++ March 2023 release Mar 9, 2023

lalitb approved these changes Mar 15, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

exporters/etw/CMakeLists.txt Show resolved Hide resolved

sdk/src/trace/batch_span_processor.cc Show resolved Hide resolved

exporters/memory/include/opentelemetry/exporters/memory/in_memory_span_exporter.h Outdated Show resolved Hide resolved

owent force-pushed the add_SpanExporter_ForceFlush branch 2 times, most recently from 0440c9e to a5be47f Compare March 16, 2023 06:35

lalitb reviewed Mar 17, 2023

View reviewed changes

owent added 9 commits March 20, 2023 10:52

+ Add ForceFlush for all LogRecordExporters and SpanExporters.

204d706

+ Optimize `BatchSpanProcessor::ForceFlush` and `BatchLogRecordProcessor::ForceFlush`. + Optimize OTLP HTTP log example + Call `ForceFlush` to prevent cancel in OTLP examples. Signed-off-by: owent <[email protected]>

Fix typo, add default implementation for SpanExporter::ForceFlush a…

c1c3f03

…nd `LogRecordExporter::ForceFlush`. Signed-off-by: owent <[email protected]>

Move constructor and destroctor of SpanExporter and `LogRecordExpor…

767821d

…ter` into .cc files. Signed-off-by: owent <[email protected]>

opentelemetry_exporter_jaeger_trace should depends //sdk/src/trace

e994c3b

Signed-off-by: owent <[email protected]>

opentelemetry_exporter_jaeger_trace should link opentelemetry_trace.

721b8d8

Signed-off-by: owent <[email protected]>

opentelemetry_exporter_etw depends opentelemetry_trace and `opent…

597205c

…elemetry_logs` Signed-off-by: owent <[email protected]>

Call TracerContext::ForceFlush when force flush or shut a tracer, f…

c9f5d4c

…ix the unit test in open-telemetry#1793 . Signed-off-by: owent <[email protected]>

Merge and fix CHANGELOG.

2f65085

Signed-off-by: owent <[email protected]>

Remove useless ForceFlush.

8359014

Signed-off-by: owent <[email protected]>

owent force-pushed the add_SpanExporter_ForceFlush branch from 9cfd097 to 8359014 Compare March 20, 2023 02:53

lalitb enabled auto-merge (squash) March 20, 2023 03:08

lalitb disabled auto-merge March 20, 2023 03:15

lalitb merged commit 9b222f2 into open-telemetry:main Mar 20, 2023

owent deleted the add_SpanExporter_ForceFlush branch March 20, 2023 06:49

marcalff changed the title ~~Add ForceFlush for all LogRecordExporters and SpanExporters.~~ [SDK] Add ForceFlush for all LogRecordExporters and SpanExporters. Apr 12, 2023

marcalff changed the title ~~[SDK] Add ForceFlush for all LogRecordExporters and SpanExporters.~~ [EXPORTER] Add ForceFlush for all LogRecordExporters and SpanExporters. Apr 12, 2023

marcalff changed the title ~~[EXPORTER] Add ForceFlush for all LogRecordExporters and SpanExporters.~~ [SDK] Add ForceFlush for all LogRecordExporters and SpanExporters. Apr 12, 2023

chuanchan1116 mentioned this pull request Apr 19, 2023

opentelemetry-cpp 1.9.0 Homebrew/homebrew-core#128783

Closed

marcalff mentioned this pull request Jun 25, 2024

[EXPORTER] OTLP HTTP session canceled after a flush, with async export #2715

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SDK] Add `ForceFlush` for all `LogRecordExporter`s and `SpanExporter`s. #2000

[SDK] Add `ForceFlush` for all `LogRecordExporter`s and `SpanExporter`s. #2000

owent commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading

lalitb left a comment

lalitb commented Mar 17, 2023

lalitb Mar 17, 2023 •

edited

Loading

owent Mar 20, 2023

lalitb Mar 20, 2023

owent commented Mar 20, 2023

[SDK] Add ForceFlush for all LogRecordExporters and SpanExporters. #2000

[SDK] Add ForceFlush for all LogRecordExporters and SpanExporters. #2000

Conversation

owent commented Feb 23, 2023 • edited Loading

Changes

codecov bot commented Feb 23, 2023 • edited Loading

Codecov Report

lalitb left a comment

Choose a reason for hiding this comment

lalitb commented Mar 17, 2023

lalitb Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

owent Mar 20, 2023

Choose a reason for hiding this comment

lalitb Mar 20, 2023

Choose a reason for hiding this comment

owent commented Mar 20, 2023

[SDK] Add `ForceFlush` for all `LogRecordExporter`s and `SpanExporter`s. #2000

[SDK] Add `ForceFlush` for all `LogRecordExporter`s and `SpanExporter`s. #2000

owent commented Feb 23, 2023 •

edited

Loading

codecov bot commented Feb 23, 2023 •

edited

Loading

lalitb Mar 17, 2023 •

edited

Loading