Enhancements to PR #86 Add minimal tracer example #94

ankit-bhargava · 2020-05-29T19:30:09Z

This PR adds build and run documentation in README.md as well as adds inline comments for the simple tracer example in PR #86 .

WillsonHG · 2020-05-29T21:31:40Z

/check-cla

pyohannes · 2020-05-30T01:40:04Z

examples/simple/main.cc

+  auto processor = std::shared_ptr<sdktrace::SpanProcessor>(new sdktrace::SimpleSpanProcessor(
+      std::move(exporter)));  // Create a Span Processor given the StdoutExporter defined earlier
+  // Initialize a trace provider with the Span Processor instance defined above
+  auto provider = nostd::shared_ptr<trace::TracerProvider>(new sdktrace::TracerProvider(processor));


I'd be more sparse with inline comments and only use them to hint at things that cannot be inferred from the code itself. Otherwise they tend to make code harder to read instead of making it easier. There's no information in

// Initialize a trace provider with the Span Processor instance defined above

that cannot be inferred from

auto provider = nostd::shared_ptr<trace::TracerProvider>(new sdktrace::TracerProvider(processor));

Reminded me on one thing that I learned long time ago - make the code talk by itself.

pyohannes · 2020-05-30T01:46:06Z

examples/simple/README.md

+
+# Simple Trace Example
+
+This example sets up a tracer in the main application then makes several calls to a custom library instrumented with the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>. Specifically, it demonstrates the setup of fundamental Open Telemetry componenets such as Tracers, Tracer Providers and Spans.  The example also illustrates context propogation from parent spans to children spans and events.  All telemetry output is directed to stdout.


I love the idea of having a README for the example. I'd prefer being more concise here. The most important things to point out:

There's a foo_library that is instrumented with the OpenTelemetry API.

There's an application in main.cc that initializes and registers a tracer provider from the OpenTelemetry SDK. Telemetry data is directed to stdout by using a custom exporter.

pyohannes · 2020-05-30T01:46:53Z

examples/simple/README.md

+bazel-bin/examples/simple/example_simple
+```
+
+## Useful Links


I think this section should be moved somewhere else. Those links aren't specific to this example but useful to the project as a whole (we wouldn't want to have the same information in all examples).

The same would apply to the Build and Run section. I think it would be better to have more general build instructions at a more central location.

I agree the links aren't relevant. Where would you suggest I move the build/run instructions? I don't think it would hurt to leave them here as well as duplicate them somewhere else as many interns like myself have never used bazel before

I would move those instructions to the CONTRIBUTING doc. Java has instructions placed there.

ankit-bhargava · 2020-06-03T21:26:46Z

I removed superfluous comments, polished the README, and moved instructions/links to CONTRIBUTING doc in PR #97

reyang · 2020-06-08T23:53:22Z

@ankit-bhargava #86 got merged, please rebase. Thanks.

linux-foundation-easycla · 2020-06-11T03:56:19Z

The committers are authorized under a signed CLA.

✅ Johannes Tax (ea487bf, 46c08e0, 3ca6a82, 445e5fb, 2b05be0)
✅ Ankit Bhargava (5b06dc1, 814e53b)

ankit-bhargava · 2020-06-11T04:01:27Z

@reyang Rebased, thanks

examples/simple/README.md

reyang · 2020-06-11T04:13:19Z

examples/simple/main.cc

  trace::Provider::SetTracerProvider(provider);
 }
 }  // namespace

 int main()
 {
-  // Removing this line will leave OT initialized with the default noop
+  // Removing this line will leave OTel SDK initialized with the default noop


This has brought an interesting topic, neither the term OT nor OTel has been officially blessed https://github.com/open-telemetry/opentelemetry-specification/search?q=OTel&unscoped_q=OTel.

I'll use "OpenTelemetry" to avoid confusion in that case.

reyang · 2020-06-11T04:29:34Z

examples/simple/README.md

+
+# Simple Trace Example
+
+In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>.  The application then calls a `foo_library` which has been manually instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>.  Resulting telemetry is directed to stdout through a custom exporter.


nit: consider using "the application" instead of "an application in main.cc"?

reyang · 2020-06-11T04:30:01Z

examples/simple/README.md

+
+# Simple Trace Example
+
+In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>.  The application then calls a `foo_library` which has been manually instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>.  Resulting telemetry is directed to stdout through a custom exporter.


Suggested change

In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>. The application then calls a `foo_library` which has been manually instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>. Resulting telemetry is directed to stdout through a custom exporter.

In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> OpenTelemetry SDK </a>. The application then calls a `foo_library` which has been manually instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>. Resulting telemetry is directed to stdout through a custom exporter.

reyang · 2020-06-11T04:31:43Z

examples/simple/README.md

+
+# Simple Trace Example
+
+In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>.  The application then calls a `foo_library` which has been manually instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>.  Resulting telemetry is directed to stdout through a custom exporter.


Remove the extra space before/after <a>/</a>. Otherwise it'll render an extra white space which is buggy.

Please use markdown links.

[Open Telemetry SDK](https://github.com/open-telemetry/opentelemetry-cpp)

reyang · 2020-06-11T04:32:06Z

examples/simple/README.md

+
+In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>.  The application then calls a `foo_library` which has been manually instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>.  Resulting telemetry is directed to stdout through a custom exporter.
+
+See <a href="https://github.com/open-telemetry/opentelemetry-cpp/blob/master/CONTRIBUTING.md"> CONTRIBUTING.md </a> for instructions on building and running the example.


Use relative link.

reyang · 2020-06-11T04:33:14Z

examples/simple/main.cc

  trace::Provider::SetTracerProvider(provider);
 }
 }  // namespace

 int main()
 {
-  // Removing this line will leave OT initialized with the default noop
+  // Removing this line will leave OpenTelemetry SDK initialized with the default noop


probably "the SDK"? - I don't know - what do you think @pyohannes?

If we want to split some hairs, leaving out this line has the effect that no SDK is used at all. All the noop stuff is in the API.

Maybe:

Removing this line will leave the default noop TracerProvider in place.

reyang · 2020-06-11T04:34:10Z

examples/simple/main.cc

  // tracer, thus being effectively deactivated.
  initTracer();

+  // Call the instrumented library


Same here, seems unnecessary.

pyohannes · 2020-06-11T05:25:34Z

examples/simple/README.md

+
+# Simple Trace Example
+
+In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>.  The application then calls a `foo_library` which has been manually instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>.  Resulting telemetry is directed to stdout through a custom exporter.


Suggested change

In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>. The application then calls a `foo_library` which has been manually instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>. Resulting telemetry is directed to stdout through a custom exporter.

In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>. The application then calls a `foo_library` which has been instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>. Resulting telemetry data is directed to stdout through a custom exporter.

manually instrumented doesn't really make sense, as we only support manual instrumentation. This might suggest we can do automatic instrumentation, but we don't.

reyang · 2020-06-13T00:00:49Z

examples/simple/README.md

+
+# Simple Trace Example
+
+In this example, the application in `main.cc` initializes and registers a tracer provider from the [OpenTelemetry SDK](https://github.com/open-telemetry/opentelemetry-cpp).  The application then calls a `foo_library` which has been instrumented using the [OpenTelemetry API](https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api).  Resulting telemetry is directed to stdout through a custom exporter.


Would you add line wrapping at column 80?

Use one space instead of two for consistency.

fixed both :)

reyang · 2020-06-13T00:03:06Z

examples/simple/main.cc

@@ -11,18 +11,19 @@ namespace
 {
 void initTracer()
 {
-  auto exporter  = std::unique_ptr<sdktrace::SpanExporter>(new StdoutExporter);
+  auto exporter = std::unique_ptr<sdktrace::SpanExporter>(new StdoutExporter);
+  // Specify the Span Processor: SimpleSpanProcessor forwards spans directly to the exporter


Span Processor -> SpanProcessor (remove the space)

I also wonder if this is self-explanatory?

agreed, removed the comment

reyang

LGTM.

reyang · 2020-06-17T17:49:54Z

This is ready to merge, @ankit-bhargava would you rebase?

g-easy · 2020-06-18T01:43:23Z

examples/simple/README.md

+# Simple Trace Example
+
+In this example, the application in `main.cc` initializes and registers a tracer
+ provider from the [OpenTelemetry SDK](https://github.com/open-telemetry/opentelemetry-cpp).


Can the leading spaces be removed?

Co-authored-by: Reiley Yang <reyang@microsoft.com>

Replacing the unofficial "OTel" shorthand with the "OpenTelemetry"

ankit-bhargava requested review from g-easy, jmacd, maxgolov, reyang, rnburn and tigrannajaryan as code owners May 29, 2020 19:30

pyohannes reviewed May 30, 2020

View reviewed changes

HudsonHumphries mentioned this pull request Jun 2, 2020

Add Useful Resources and Example Build Instructions to CONTRIBUTING.md #97

Merged

reyang reviewed Jun 11, 2020

View reviewed changes

examples/simple/README.md Outdated Show resolved Hide resolved

reyang reviewed Jun 11, 2020

View reviewed changes

pyohannes reviewed Jun 11, 2020

View reviewed changes

ankit-bhargava mentioned this pull request Jun 12, 2020

REQUEST: New membership for abharg open-telemetry/community#384

Closed

pyohannes approved these changes Jun 12, 2020

View reviewed changes

reyang reviewed Jun 13, 2020

View reviewed changes

reyang added the documentation Improvements or additions to documentation label Jun 16, 2020

ankit-bhargava requested a review from a team June 17, 2020 14:58

reyang approved these changes Jun 17, 2020

View reviewed changes

g-easy reviewed Jun 18, 2020

View reviewed changes

Johannes Tax and others added 12 commits June 19, 2020 01:13

Add a simple example for the tracer workflow

1ef8430

Remove obsolete file

4d94891

PR comments

f61ae52

Bazel fixes

3065c6b

Fix bazel build

d78bf3e

Adding documentation and build instructions

f2181b9

Condensed comments, moved build/run instructions to CONTRIBUTING.md

52424b3

Typo fix in README.md

5512d5f

Co-authored-by: Reiley Yang <reyang@microsoft.com>

Updating comments in main.cc

72f9c18

Replacing the unofficial "OTel" shorthand with the "OpenTelemetry"

Incorporating review suggestions

7c32c86

spacing consistency and line wrapping

6ae95bb

rebased and removed leading spaces

e89e017

reyang merged commit c86794e into open-telemetry:master Jun 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancements to PR #86 Add minimal tracer example #94

Enhancements to PR #86 Add minimal tracer example #94

ankit-bhargava commented May 29, 2020

WillsonHG commented May 29, 2020

pyohannes May 30, 2020

reyang Jun 11, 2020 •

edited

Loading

pyohannes May 30, 2020

pyohannes May 30, 2020

ankit-bhargava Jun 1, 2020

pyohannes Jun 1, 2020

ankit-bhargava commented Jun 3, 2020

reyang commented Jun 8, 2020

linux-foundation-easycla bot commented Jun 11, 2020 •

edited

Loading

ankit-bhargava commented Jun 11, 2020

reyang Jun 11, 2020

ankit-bhargava Jun 11, 2020

reyang Jun 11, 2020

reyang Jun 11, 2020

reyang Jun 11, 2020

pyohannes Jun 11, 2020

reyang Jun 11, 2020

reyang Jun 11, 2020

pyohannes Jun 11, 2020

reyang Jun 11, 2020

pyohannes Jun 11, 2020

pyohannes Jun 11, 2020

reyang Jun 13, 2020

reyang Jun 13, 2020

ankit-bhargava Jun 17, 2020

reyang Jun 13, 2020

ankit-bhargava Jun 17, 2020

reyang left a comment

reyang commented Jun 17, 2020

g-easy Jun 18, 2020


		# Simple Trace Example

		This example sets up a tracer in the main application then makes several calls to a custom library instrumented with the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>. Specifically, it demonstrates the setup of fundamental Open Telemetry componenets such as Tracers, Tracer Providers and Spans. The example also illustrates context propogation from parent spans to children spans and events. All telemetry output is directed to stdout.


		# Simple Trace Example

		In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>. The application then calls a `foo_library` which has been manually instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>. Resulting telemetry is directed to stdout through a custom exporter.


		In this example, an application in `main.cc` initializes and registers a tracer provider from the <a href="https://github.com/open-telemetry/opentelemetry-cpp"> Open Telemetry SDK </a>. The application then calls a `foo_library` which has been manually instrumented using the <a href="https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api"> OpenTelemetry API </a>. Resulting telemetry is directed to stdout through a custom exporter.

		See <a href="https://github.com/open-telemetry/opentelemetry-cpp/blob/master/CONTRIBUTING.md"> CONTRIBUTING.md </a> for instructions on building and running the example.


		# Simple Trace Example

		In this example, the application in `main.cc` initializes and registers a tracer provider from the [OpenTelemetry SDK](https://github.com/open-telemetry/opentelemetry-cpp). The application then calls a `foo_library` which has been instrumented using the [OpenTelemetry API](https://github.com/open-telemetry/opentelemetry-cpp/tree/master/api). Resulting telemetry is directed to stdout through a custom exporter.

Enhancements to PR #86 Add minimal tracer example #94

Enhancements to PR #86 Add minimal tracer example #94

Conversation

ankit-bhargava commented May 29, 2020

WillsonHG commented May 29, 2020

Choose a reason for hiding this comment

reyang Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankit-bhargava commented Jun 3, 2020

reyang commented Jun 8, 2020

linux-foundation-easycla bot commented Jun 11, 2020 • edited Loading

ankit-bhargava commented Jun 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reyang left a comment

Choose a reason for hiding this comment

reyang commented Jun 17, 2020

Choose a reason for hiding this comment

reyang Jun 11, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jun 11, 2020 •

edited

Loading