WIP TraceZ Processor by jajanet · Pull Request #5 · kmanghat/opentelemetry-cpp

jajanet · 2020-07-06T19:02:52Z

Progress includes:

Processor code, which interfaces with the original code and has working added methods relevant to TraceZ. Notably, the methods hold running spans and newly completed spans (completed spans that have been sent before are cleared)
Comprehensive tests that test the functionality described above in different scenarios, including for cumulative and non-cumulative situations. Checking individual span information has been temporary commented out.
Bazel and CMake build files

dustinpho · 2020-07-07T15:21:11Z

+   * Shut down the processor and do any cleanup required. Ended spans are
+   * exported before shutdown. After the call to Shutdown, subsequent calls to
+   * OnStart, OnEnd, ForceFlush or Shutdown will return immediately without
+   * doing anything.


Should there be a flag to signal if Shutdown is called so it can exit early in those functions?

Would it be better to throw an exception if any of those are called after Shutdown is called?

Yeah I can add that, thanks!! I also made the functions return immediately instead, based on the doc comments

dustinpho · 2020-07-07T15:47:17Z

+       completed_spans_.push_back(std::move(span));
+     }
+  }
+


One option is to collapse these two functions into one, and return some struct that bundles copies of the running and completed spans together.

struct RunningAndCompletedSpans { <> running_spans_; <> completed_spans_; }; RunningAndCompletedSpans GetRunningAndCompletedSpansSnapshot() noexcept { auto completed_spans = std::move(completed_spans_); completed_spans.clear(); return {std::unordered_set<Recordable*>(running_spans_), completed_spans}; }

Something like that, that way it'll just give the Aggregator a consistent view while it's aggregating the spans.

Okay, that sounds good! I should replace the running and completed getters in favor of returning them all, since the main goal is giving this information in a consistent manner and the aggregator should change its methods accordingly?

dustinpho · 2020-07-07T15:52:49Z

+
+ private:
+  std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> exporter_;
+  std::unordered_set<opentelemetry::sdk::trace::Recordable*> running_spans_;


Is the TraceZProcessor supposed to be a thread-safe class (do multiple threads share a single TraceZProcessor, or do they all have their own)?

I think it's supposed to be thread-safe and hypothetical threads could share a single processor if I'm looking the spec correctly https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#interface-definition

I believe usually reading and writing spans in storage would be in the exporter, so adding thread safety isn't a mentioned for the processor and we would need it?

liadavid

Looks good, Nice tests! Some small comments.

liadavid · 2020-07-08T00:27:25Z

+
+  /**
+   * Initialize a span processor.
+   * @param exporter the exporter used by the span processor. Here, the purpose


Nit: Remove 'exporter' comment

liadavid · 2020-07-08T00:28:14Z

+  explicit TracezSpanProcessor() noexcept {}
+
+  /**
+   * Create a span recordable using the associated exporter.


Nit: Update comment

liadavid · 2020-07-08T00:40:22Z

+
+
+/*
+ * Helper function to create a processor when the type of exporter doesn't


Nit: remove exporter

liadavid · 2020-07-08T00:40:43Z

+
+
+/*
+ * Helper function uses the current processor tov update spans contained in completed_spans


Nit: typo: tov -> to

liadavid · 2020-07-08T00:44:30Z

+    completed.clear();
+    completed = std::move(spans.completed);
+  }
+  else {


Nit: It should be:
if () {
...
} else {
...
}

See: https://google.github.io/styleguide/cppguide.html#Conditionals

liadavid · 2020-07-08T00:50:01Z

+TEST(TracezSpanProcessor, NoSpans) {
+  auto processor = MakeProcessor();
+  auto spans = processor->GetSpanSnapshot();
+  auto recordable = processor->MakeRecordable();


'recordable' is not used here.

liadavid · 2020-07-08T01:16:09Z

+namespace zpages
+{
+/**
+ * The span processor passes finished recordables to an exporter and is


Please update comment

liadavid · 2020-07-08T01:21:12Z

+   * @return snapshot of all currently running spans and newly completed spans
+   * at the time that the function is called
+   */
+  CollectedSpans GetSpanSnapshot() noexcept;


Worth adding a TODO: Verify/handle a case when GetSnapshot is called when spans start/end to run.

This should be covered by copy on write. While we plan to add it, it is good to "declare" that it is our plan.

…into zpages-tracez-processor1

This reverts commit eb81ff6.

…t/opentelemetry-cpp into zpages-tracez-processor1

…/kmanghat/opentelemetry-cpp into zpages-tracez-processor1" This reverts commit 38cc77a, reversing changes made to 2d061af.

jajanet · 2020-07-08T22:36:00Z

See WIP PR for the main repo: open-telemetry#164

Consolidate tracez processor changes

1147049

jajanet changed the title ~~Consolidate tracez processor changes~~ WIP TraceZ Processor Jul 6, 2020

jajanet requested review from kmanghat and liadavid July 6, 2020 21:37

jajanet added 4 commits July 6, 2020 22:44

Add some doc info

f721a1c

Add test helper function and cleaning

51b16e9

Ensure completed is empty if no new spans

b671912

Add nullptr check

d69a2b3

dustinpho reviewed Jul 7, 2020

View reviewed changes

jajanet added 3 commits July 7, 2020 18:36

Remove exporter, add span struct, shutdown signal

f32cf66

Add todo notes

3f09ffb

Add space for formatting

1a0875f

liadavid reviewed Jul 8, 2020

View reviewed changes

jajanet added 7 commits July 8, 2020 14:44

Update comments/shutdown/flush, add TODOs

af4a6d8

Use timeout for shutdown and flush

9e556f8

Move timeout line

eb81ff6

Merge branch 'master' of https://github.com/kmanghat/opentelemetry-cpp …

2a8696e

…into zpages-tracez-processor1

Revert "Move timeout line"

2d061af

This reverts commit eb81ff6.

Merge branch 'zpages-tracez-processor1' of https://github.com/kmangha…

38cc77a

…t/opentelemetry-cpp into zpages-tracez-processor1

Revert "Merge branch 'zpages-tracez-processor1' of https://github.com…

aa1b39c

…/kmanghat/opentelemetry-cpp into zpages-tracez-processor1" This reverts commit 38cc77a, reversing changes made to 2d061af.

jajanet closed this Jul 8, 2020



		/*
		* Helper function to create a processor when the type of exporter doesn't



		/*
		* Helper function uses the current processor tov update spans contained in completed_spans

Conversation

jajanet commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jajanet Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jajanet Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liadavid left a comment

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!

jajanet commented Jul 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jajanet commented Jul 6, 2020 •

edited

Loading

jajanet Jul 7, 2020 •

edited

Loading

jajanet Jul 7, 2020 •

edited

Loading