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

Falcon support #1136

Closed
ioquatix opened this issue Aug 6, 2020 · 12 comments
Closed

Falcon support #1136

ioquatix opened this issue Aug 6, 2020 · 12 comments
Assignees
Labels
community Was opened by a community member feature-request A request for a new feature or change to an existing one integrations Involves tracing integrations

Comments

@ioquatix
Copy link
Contributor

ioquatix commented Aug 6, 2020

Hello. I'm the author of Falcon and I'm hoping to add support for DataDog's tracing.

1/ Have you tested DataDog with Falcon?
2/ Are you interested in helping with this?

@marcotc
Copy link
Member

marcotc commented Aug 6, 2020

Hey @ioquatix, thank you for all your work on Ruby asynchronous I/O!

We have not tested ddtrace with Falcon yet, but we have interest in supporting it.
We haven't received any user feedback (until now) asking for Falcon instrumentation, or instrumentation for any other async I/O gem for that matter. But we are aware that of the progress in Ruby towards async I/O solutions, as we have already seen in other languages we support here at Datadog.

From a technical standpoint, implementing Falcon instrumentation only carries one caveat around ensuring ddtrace thread-local data structures stay consistent. But otherwise, instrumenting the interesting parts of Falcon (and/or its dependencies: nio4r, async-io, async, etc.) by creating a Span around the relevant code paths would likely suffice to provide visibility into the framework.

That being said, our team is not currently focused on working on this new instrumentation, but we are more than happy to assist with this work and help with any blockers necessary to get it merged.

@marcotc marcotc added community Was opened by a community member feature-request A request for a new feature or change to an existing one labels Aug 6, 2020
@ioquatix
Copy link
Contributor Author

ioquatix commented Aug 7, 2020

Can you tell me how I could add tests to falcon? e.g. some high level integration tests which run some requests, log the spans to a local file or something, and then I can check they were captured correctly.

@marcotc
Copy link
Member

marcotc commented Sep 14, 2020

👋 @ioquatix, had some dedicated time to try to write some specs you could follow, but I realized, after spending some time with the Async library, that our existing instrumentation dependent on thread-local variables would likely not work as expected with Falcon.

A Fiber#yield today would maintain the instrumentation context, meaning any code executed in other fibers would get assign as children of the caller of Fiber#yield. I can foresee this yielding a highly intertwined result.

The code responsible for the thread-local behaviour is encapsulated in this class:

def local=(ctx)
Thread.current[@key] = ctx
end
# Return the thread-local context.
def local
Thread.current[@key] ||= Datadog::Context.new
end

The implementation of a Fiber-aware ContextProvider doesn't seem too complicated, the main challenge being to ensure no behaviour change for non-Fiber users.

I think this is the very first challenge we face to implement support for users of Async.

@ioquatix
Copy link
Contributor Author

ioquatix commented Sep 15, 2020

Just gonna drop the bomb. Did you know Thread.current#[] is actually fiber local?

@marcotc
Copy link
Member

marcotc commented Sep 15, 2020

No, I did not! I thought it was only the other way around... thank you for that!

I'll try next to find what level we would like to instrument Falcon: I noticed while working with it yesterday that a lot of the heavy lifting is done by other "Socketry", and we might have better luck with those.

@ioquatix
Copy link
Contributor Author

I have a plan to expose instrumentation hooks.

This is APM agnostic. I don't want APM libraries injecting random stuff into the async code if possible. I'd rather have a well defined interface for it.

I don't know what that is yet, but interested to hear how you get on.

@delner delner added the integrations Involves tracing integrations label Apr 21, 2021
@ioquatix
Copy link
Contributor Author

Okay, I've created my agnostic hooks:

https://github.com/socketry/traces

I've implemented a backend for Datadog:

https://github.com/socketry/traces-backend-datadog

I've added some test trace captures including distributed tracing to https://github.com/socketry/async-http.

This would provide a decent coverage within Falcon, at least initially. But there are some wider issues including metrics, and this is all largely untested. Are you able to give me some credit on my plan so I can test it further?

image

@ioquatix
Copy link
Contributor Author

By the way, with the advent of the above gems, this feature request is no longer relevant since I'll directly provide the tracing/metrics/integrations rather than expecting this gem to provide them.

@ivoanjo
Copy link
Member

ivoanjo commented Oct 18, 2021

This would provide a decent coverage within Falcon, at least initially. But there are some wider issues including metrics, and this is all largely untested. Are you able to give me some credit on my plan so I can test it further?

Hey!

I was asking internally and there's actually a program at Datadog to give away free (as in beer/gratis) accounts for relevant open-source projects, which fits really well with Falcon and the Socketry ecosystem.

Do you mind filling https://www.datadoghq.com/partner/open-source/ and dropping a note in the comments that you've been working on this with me? We should be able to get you setup nicely :)

@ioquatix
Copy link
Contributor Author

@ivoanjo I am still waiting, should I have heard something by now? Thanks!

@ivoanjo
Copy link
Member

ivoanjo commented Oct 25, 2021

I shall find out what's up! :)

@ivoanjo
Copy link
Member

ivoanjo commented Oct 26, 2021

Should be all set now! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member feature-request A request for a new feature or change to an existing one integrations Involves tracing integrations
Projects
None yet
Development

No branches or pull requests

4 participants