Added feature flag for asynchronous export by DebajitDas · Pull Request #1295 · open-telemetry/opentelemetry-cpp

DebajitDas · 2022-03-29T08:59:03Z

Fixes # Adds feature flag for asynchronous export.
Changes for max export and wait on async export not triggered by shutdown/force-flush are pending

Changes

Please provide a brief description of the changes here.

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

lalitb · 2022-03-30T05:05:47Z

LGTM in general, @owent may want to have a look into this, specifically the batch span/log processor part. Also, we should add a CI action to test the async feature ( e.g, add another test similar to bazel_test which builds using ENABLE_ASYNC_EXPORT macro).

owent · 2022-03-30T06:05:56Z

LGTM in general, @owent may want to have a look into this, specifically the batch span/log processor part. Also, we should add a CI action to test the async feature ( e.g, add another test similar to bazel_test which builds using ENABLE_ASYNC_EXPORT macro).

Agree, I think we can just add -DENABLE_ASYNC_EXPORT for bazel options and add -DENABLE_ASYNC_EXPORT=ON for cmake options in do_ci.sh/do_ci.ps1

owent · 2022-03-30T05:51:07Z

      const nostd::span<std::unique_ptr<opentelemetry::sdk::trace::Recordable>> &spans) noexcept
      override;

+#ifdef ENABLE_ASYNC_EXPORT


Could you please also put max_concurrent_requests in OtlpHttpExporterOptions into ENABLE_ASYNC_EXPORT ? It's useless when async exporting is disabled.

owent · 2022-03-30T05:52:08Z

      const nostd::span<std::unique_ptr<opentelemetry::sdk::logs::Recordable>> &records) noexcept
      override;

+#  ifdef ENABLE_ASYNC_EXPORT


Could you please also remove max_concurrent_requests in OtlpHttpExporterOptions and OtlpHttpLogExporterOptions when we do not decalre ENABLE_ASYNC_EXPORT ? It's useless when async exporting is disabled.

owent · 2022-03-30T06:03:14Z


+option(WITH_ASYNC_EXPORT_PREVIEW "Whether enable async export" OFF)
+
+if(WITH_ASYNC_EXPORT_PREVIEW)


We should also add INTERFACE definitions into api/CMakeLists.txt.

if(WITH_LOGS_PREVIEW) target_compile_definitions(opentelemetry_api INTERFACE ENABLE_ASYNC_EXPORT) endif()

So that when we use optentelemetry-cpp as a cmake module(find_package(opentelemetry-cpp CONFIG)) , this definition can be auto added into all targets that direct or indirectly depend on opentelemetry-cpp.

I find there are severval missing definitions in api/CMakeLists.txt and be add by add_definitions(). All definitions should be added by target_compile_definitions(opentelemetry_api INTERFACE ...) to be expoted by cmake.
We should avoid to use add_definitions(...) unless the definitions is only used by unit tests, should I create another issue to fix it? @lalitb @ThomsonTan

owent · 2022-03-30T06:09:46Z

         * logged in this case. */
      }
    }
+#ifdef ENABLE_ASYNC_EXPORT


Should if (is_export_async_ == false) only be checked when ENABLE_ASYNC_EXPORT is defined?

owent · 2022-03-30T06:14:53Z

                               bool is_export_async = false) noexcept
-      : exporter_(std::move(exporter)), is_export_async_(is_export_async)
+      : exporter_(std::move(exporter)),
+#ifdef ENABLE_ASYNC_EXPORT


is_export_async will not be declared without ENABLE_ASYNC_EXPORT in BatchSpanProcessorOptions. I think we should keep the same behaviour with SimpleSpanProcessor and just remove is_export_async_.

owent · 2022-03-30T06:16:47Z

      max_queue_size_(max_queue_size),
      scheduled_delay_millis_(scheduled_delay_millis),
      max_export_batch_size_(max_export_batch_size),
+#  ifdef ENABLE_ASYNC_EXPORT


is_export_async will not be declared without ENABLE_ASYNC_EXPORT in BatchSpanProcessorOptions. I think we should also remove is_export_async_ here.

owent · 2022-03-30T06:20:06Z

   * Determines whether the export happens asynchronously.
   * Default implementation is synchronous.
   */
  bool is_export_async = false;


Should we also remove is_export_async_ in BatchLogProcessor and BatchSpanProcessor ?

owent · 2022-03-30T06:25:14Z

      /* Alert user of the failed export */
    }
  }
+#  ifdef ENABLE_ASYNC_EXPORT


Just like in SimpleSpanProcessor, if (is_export_async_ == false) should only be checked when ENABLE_ASYNC_EXPORT is defined, or we should always call sync Export.

owent · 2022-03-30T06:28:55Z

          nostd::span<std::unique_ptr<Recordable>>(spans_arr.data(), spans_arr.size()));
      NotifyCompletion(notify_force_flush, synchronization_data_);
    }
+#ifdef ENABLE_ASYNC_EXPORT


Just like in SimpleSpanProcessor, if (is_export_async_ == false) should only be checked when ENABLE_ASYNC_EXPORT is defined, or we should always call sync Export.

owent · 2022-03-30T06:29:14Z

          nostd::span<std::unique_ptr<Recordable>>(records_arr.data(), records_arr.size()));
      NotifyCompletion(notify_force_flush, synchronization_data_);
    }
+#  ifdef ENABLE_ASYNC_EXPORT


Just like in SimpleSpanProcessor, if (is_export_async_ == false) should only be checked when ENABLE_ASYNC_EXPORT is defined, or we should always call sync Export.

owent

LGTM.
The rest of the things is adding CI actions to test the async feature.(with and without)

DebajitDas · 2022-03-31T02:35:10Z

Could this PR be merged? I would add max export changes once this is merged.

owent · 2022-03-31T02:52:26Z

@lalitb I think this PR can be merged right now. I'm not in @open-telemetry/cpp-approvers right now, my approval can not let the process continue.

lalitb · 2022-03-31T04:24:22Z

We should avoid to use add_definitions(...) unless the definitions is only used by unit tests, should I create another issue to fix it? @lalitb @ThomsonTan

Good point. Yes please create a issue to fix it @owent

lalitb · 2022-03-31T04:24:56Z

@lalitb I think this PR can be merged right now. I'm not in @open-telemetry/cpp-approvers right now, my approval can not let the process continue.

Thanks, Have added you to approvers now. Seems got missed somehow.

lalitb · 2022-03-31T04:26:17Z

@DebajitDas - Merging this, the CI test and max_export can be added in separate PR.

Added feature flag for export async

454dade

DebajitDas requested a review from a team March 29, 2022 08:59

owent requested changes Mar 30, 2022

View reviewed changes

Incorporated Review Comments

510a6a3

owent approved these changes Mar 30, 2022

View reviewed changes

lalitb merged commit ad3bdfe into open-telemetry:async-changes Mar 31, 2022


		option(WITH_ASYNC_EXPORT_PREVIEW "Whether enable async export" OFF)

		if(WITH_ASYNC_EXPORT_PREVIEW)

Uh oh!

Conversation

DebajitDas commented Mar 29, 2022

Changes

Uh oh!

lalitb commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owent commented Mar 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

owent left a comment

Choose a reason for hiding this comment

Uh oh!

DebajitDas commented Mar 31, 2022

Uh oh!

owent commented Mar 31, 2022

Uh oh!

lalitb commented Mar 31, 2022

Uh oh!

lalitb commented Mar 31, 2022

Uh oh!

lalitb commented Mar 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lalitb commented Mar 30, 2022 •

edited

Loading