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 TraceWriter and AsyncTraceWriter workers #986

Merged
merged 1 commit into from
Mar 31, 2020

Conversation

delner
Copy link
Contributor

@delner delner commented Mar 30, 2020

Extracted from #879

This pull request adds the Datadog::Workers::TraceWriter and Datadog::Workers::AsyncTraceWriter classes, which act as replacements for the Datadog::Writer class.

  • The TraceWriter is the base class for trace writing and defines how traces are written to a transport; it's analogous to SyncWriter and replaces it.
  • AsyncTraceWriter extends TraceWriter, decorating it with asynchronous behaviors that allows the same trace writing behavior to execute in its own worker thread on a polling basis. It will replace Datadog::Writer and the AsyncTransport.

Once in place, the goal is to have these workers act as drop-in replacements for the writer, then eventually phase out/remove the Writer and AsyncTransport entirely, in a future PR.

@delner delner added core Involves Datadog core libraries dev/refactor Involves refactoring existing components labels Mar 30, 2020
@delner delner requested review from marcotc, brettlangdon and a team March 30, 2020 17:03
@delner delner self-assigned this Mar 30, 2020

# Writes traces to transport asynchronously,
# using a thread & buffer.
class AsyncTraceWriter < TraceWriter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I move this to its own file? Was useful seeing both of these together, but it might make sense to have one class per file whenever possible/reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a problem with having them both in this file right now, specially since one inherits from the other, making navigating the implementation of AsyncTraceWriter easier.

If sync and async implementation become more divergent we could move them to separate files with little hassle.

@delner delner added this to In review in Active work Mar 30, 2020
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Very clean PR to review. Awesome work on splitting the work across many PRs @delner! 🚀🚀🚀

@delner delner merged commit e785026 into master Mar 31, 2020
Active work automation moved this from In review to Merged & awaiting release Mar 31, 2020
@delner delner deleted the feature/add_trace_writer branch March 31, 2020 19:45
@delner delner added this to the 0.35.0 milestone Mar 31, 2020
@marcotc marcotc moved this from Merged & awaiting release to Released in Active work May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries dev/refactor Involves refactoring existing components
Projects
Active work
  
Released
Development

Successfully merging this pull request may close these issues.

None yet

2 participants