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 http client/server example #131

Merged
merged 4 commits into from
Nov 12, 2019
Merged

Add http client/server example #131

merged 4 commits into from
Nov 12, 2019

Conversation

elskwid
Copy link
Member

@elskwid elskwid commented Oct 23, 2019

Overview

Adds a simple http client/server example using a Sinatra server and Faraday client. The example is modeled after the Go http example.

Note: the console exporter won't be called until we merge in the in_span fix from #118

Details

  • Adds the examples top-level directory
  • Adds an http example directory that contains the client, server, and a small helper script
  • Adds the ex-http docker-compose server to simplify setup/running of the example
  • Adds a README to walk a person through running the example

Depends on:

This PR depends on #144 to function but it can be merged in any order.

Finishes

#130 - Add simple HTTP client/server example

@elskwid elskwid changed the title Dm/examples add http Add http client/server example Oct 23, 2019
@elskwid
Copy link
Member Author

elskwid commented Oct 23, 2019

I added a note that this requires the fix from #118, which I'm working on next.

examples/http/client.rb Outdated Show resolved Hide resolved
exporter = Examples::Exporters::Console.new
processor = SDK::Trace::Export::SimpleSpanProcessor.new(exporter)
tracer = OpenTelemetry.tracer_factory.tracer('http.client', 'semver:1.0')
tracer.add_span_processor(processor)
Copy link
Contributor

Choose a reason for hiding this comment

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

So much wiring :-(

examples/http/client.rb Outdated Show resolved Hide resolved
examples/http/server.rb Outdated Show resolved Hide resolved
examples/http/server.rb Outdated Show resolved Hide resolved
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

This is a good start. It definitely exposes that we could have some better ergonomics around setup and initialization.

examples/http/client.rb Outdated Show resolved Hide resolved
examples/http/server.rb Outdated Show resolved Hide resolved
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

I think that our client/server examples should demonstrate how to handle context propagation at process boundaries. So, the server example should show the use of extract to extract a span context and parent a span in process. The client example should use inject to inject a span context into the http headers of the outbound request.

For the server example, I think it'd make sense to demonstrate it using a Rack middleware as it's a realistic example of where a user will be at a request boundary.

For the client example, we can stick with the manual request with Faraday, but add a step to inject the current span's context into the outbound request.

@mwear
Copy link
Member

mwear commented Oct 24, 2019

It's possible that we could de-grapify the example from: #132 and use it here.

@fbogsany
Copy link
Contributor

I completely agree with Matt. A simple example with Rack middleware to extract + creation of a child span in the server, and root span + Faraday + inject in the client is 💯

@elskwid
Copy link
Member Author

elskwid commented Oct 25, 2019

@mwear @fbogsany thanks for the feedback. I'll get the direct code feedback in here today/this weekend. As far as the context propagation - I mentioned that in the PR description, I would LOVE to have that in there but it isn't really clear how ours works. I guess it's the 🐔 + 🥚 problem of having no examples while writing the examples. 😄 Any chance I could get some guidance on propagation?

I think there's some usefulness in having really really simple examples. I'm not sure if that means this example should use Rack middleware or not. I realize that most people looking at our examples may be more than passingly familiar with tracing concepts but as a complete newbie I found the Go example to be very clear. Having said all that, I'm happy to take this in the direction you think it should go.

@elskwid
Copy link
Member Author

elskwid commented Oct 25, 2019

@mwear @fbogsany just to add a little bit of background to my question about context propagation:

The docs for inject say that the method "sets" the context on the carrier but the method really looks to just yield carrier, header, and context which leads me to believe the setting needs to be part of that block for the specific carrier. In my case, I need to write the headers to the request on the client-side (and do the actual extraction on the server-side).

It looks like other libraries have propagators that know how to interact with the carrier. Does that sound right? Are we planning to create propagators for typical use cases?

@mwear
Copy link
Member

mwear commented Oct 25, 2019

@elskwid Currently there is an HTTPTextFormat propagator. The API is based off of this specification. The blocks play the role of the getter and setter in that document.

This is a diversion from OpenTracing, where inject and extract know how to get or set the context in the carrier. Your examples have shown us where our APIs could use some improvement previously, I'm curious if you can point out the differences you see with other languages here.

As far as distributed context goes, we don't yet have an implementation for the SDK. This is something that is very likely to change in light of context propagation OTEP.

Copy link
Contributor

@fbogsany fbogsany left a comment

Choose a reason for hiding this comment

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

One small change request. Otherwise this LGTM!

examples/http/server.rb Outdated Show resolved Hide resolved
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

I tentatively approve of this PR, but we should get the context piece worked out before merging it.

examples/http/server.rb Outdated Show resolved Hide resolved
@elskwid
Copy link
Member Author

elskwid commented Nov 6, 2019

@mwear @fbogsany I believe this is good to go based on your feedback. @mwear can you take a look at my usage of context in the server and make sure it tracks with your thinking? This won't work until #144 is merged but code-wise, it's there.

examples/http/server.rb Outdated Show resolved Hide resolved
examples/http/server.rb Outdated Show resolved Hide resolved
span.set_attribute('http.method', env['REQUEST_METHOD'])
span.set_attribute('http.route', env['PATH_INFO'])
span.set_attribute('http.url', env['REQUEST_URI'])
span.add_event(name: 'handle http.server.get')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a slightly odd example. A more realistic one might be the size of the response body (after the @app.call).

Copy link
Member Author

Choose a reason for hiding this comment

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

@fbogsany as an event? Okay, I was reading the name event literally as in, this thing happened at this time.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true, but for this particular operation, that's implicitly the start of the span, so the event isn't that interesting.

I like this blog post as an example of events in a trace (look at the pictures rather than the text): https://medium.com/google-cloud/tracing-google-cloud-8f0c8ba8181c. There, the events are informative little snippets about things that occurred during the span. Some of them may be numeric values (like 146 bytes sent and 40 bytes received) while others are things like DNSStart and DNSDone to record when DNS lookup happened. The latter could conceivably be a span itself, but doesn't have to be.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my larger point here is that events should typically be used to add colour to the span, and this specific one doesn't really do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@fbogsany thank you for all the context. I removed the events to clarify the example and keep it relatively simple.

examples/http/client.rb Outdated Show resolved Hide resolved
@elskwid
Copy link
Member Author

elskwid commented Nov 7, 2019

@mwear @fbogsany I've updated after the latest round of feedback. Thank you!

Don Morrison added 2 commits November 8, 2019 00:41
Adds simple client and server examples using a console exporter to
demonstrate instrumentation.
Copy link
Contributor

@fbogsany fbogsany left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@fbogsany fbogsany merged commit 1bc697c into open-telemetry:master Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants