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

Code examples: HTTP server (grape) instrumentation example #132

Closed
wants to merge 3 commits into from
Closed

Code examples: HTTP server (grape) instrumentation example #132

wants to merge 3 commits into from

Conversation

duonoid
Copy link
Contributor

@duonoid duonoid commented Oct 23, 2019

Overview

This is a standalone http server example using grape. It helps toward #97 and should work in conjunction with #130/#131.

The intention is to demonstrate (as correctly as possible), the usage of the api/sdk as middleware.

Details

  • Adds examples/http/server/grape/, to demonstrate 'http server' wiring
  • Adds 'ex-grape' docker-compose service, for running in isolated environment
  • Example runs on port 4568, so it can run alongside another example server

* for debug/development/exploratory output
* use pp for readability
* example usage:
  $ docker-compose run --rm --service-ports ex-grape
  $ curl "localhost:4568/test"

* Expected:
  SpanData is output to console
@duonoid duonoid marked this pull request as ready for review October 23, 2019 18:22
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 am 100% onboard with the client and server demos, that generically show how someone would go about writing instrumentation for a client or server. I wonder about the value of having a demo showing how to instrument a framework that there is going to be auto-instrumentation for such as Grape. Especially since the approach in this PR is very different from the Datadog Grape instrumentation that we are supposed to be converting over. The Datadog instrumentation is using ActiveSupport::Notifications and monkey patching, where as this example uses a Rack middleware.

For reference the Datadog Grape instrumentation is implemented in the following files:

The instrumentation is (auto) installed using the Datadog patcher:

I think it will be confusing to include this example when the Grape auto-instrumentation is likely going to look very different. I'm definitely open to hearing other thoughts though.

@mwear
Copy link
Member

mwear commented Oct 24, 2019

This got me thinking about our examples a little more and I made this comment: #131 (review).

I think the middleware from this example could be converted to a generic rack middleware and used in the server example. While it's true that there will also be Rack auto-instrumentation, I think a Rack middleware is an ideal place to demonstrate how to extract context from an incoming request and use it to parent spans in-process.

Again, I'm open to any other thoughts or ideas.

@elskwid
Copy link
Member

elskwid commented Oct 25, 2019

@mwear Do you think there is any value in having examples that show the auto-instrumentation versus just having people go look at the actual implementation?

@duonoid and I used the DataDog code as a way to understand how tracing is actually being used at the code level and we felt like it would be helpful to contribute that code back. For example, I have a version of my http client/server example (#131) that uses middleware and Sinatra hooks for the server and middleware for the Faraday client - exactly as the DataDog library does.

Would it better if we just chalked those up to learning and good reference? It's fine either way since there was very real value in coding up the examples. I totally understand not all example code belongs in the examples for the project. 😁

@duonoid
Copy link
Contributor Author

duonoid commented Oct 25, 2019

It seems like there are a couple of ways forward:

  • extract a more generic middleware implementation, for use in Add http client/server example #131
  • change 'grape' example to more closely resemble the current datadog implementation

@mwear
Copy link
Member

mwear commented Oct 25, 2019

I don't think I should have the last say in this, but here are my thoughts.

If #131 has a Rack Middleware example, then there is not much to be gained from also having this example in the repo. The generic examples will help folks as a starting point for instrumenting web frameworks and http clients that aren't auto-instrumented.

I also don't think there is much value in another grape example that is more similar to the grape auto-instrumentation as we could just point to the grape auto-instrumentation as the example.

@mwear
Copy link
Member

mwear commented Oct 30, 2019

We can try to reuse some of this work in #131. Closing this as we discussed at the SIG meeting.

@mwear mwear closed this Oct 30, 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