Skip to content

Fix distributed tracing #28

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

Merged
merged 5 commits into from
Feb 11, 2021
Merged

Fix distributed tracing #28

merged 5 commits into from
Feb 11, 2021

Conversation

agocs
Copy link
Contributor

@agocs agocs commented Feb 11, 2021

What does this PR do?

This provides methods for getting the datadog trace and span ID from incoming request headers and setting them inside dd-agent-java attached to the JVM. Specifically:

  • in DDLambda.java, I've added startSpan. startSpan uses the OpenTracing API to communicate with dd-agent-java to start a new span. If the appropriate headers are set on the request, startSpan packages that information in such a way that dd-agent-java can read the headers and set the traceID and parentID on that span.
  • I've also added finish, which stops the active span and closes the tracer scope opened by startSpan. finish is a noop if no span has been started. For the time being, users still must instantiate a new DDLambda([request, ]context) in order to start a span and call DDLambda.finish() in order to finish the span.
  • I've also added a checkTraceEnabled function that checks the environment variables to see if the user has set DD_TRACE_ENABLED to true or not.

I'm intentionally not updating the README since we're keeping the dd-agent-java lambda layer private for now. Will update the README with documentation once we choose to GA dd-agent-java.

Does not include logic to merge x-ray traces yet.

Motivation

Testing Guidelines

Additional Notes

Types of changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Checklist

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog

@agocs agocs requested a review from a team as a code owner February 11, 2021 15:10
/**
* Finish the active span. If you have installed the dd-trace-java Lambda
* layer, you MUST call DDLambda.finish() at the end of your Handler
* in order to finish spans.
Copy link
Contributor

@nhinsch nhinsch Feb 11, 2021

Choose a reason for hiding this comment

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

In the other libraries, we are able to run code automatically at the end of the function execution to finish the span (and do other stuff, like send enhanced metrics). Is it not possible to do that in Java as well? I think requiring customers to do it manually could potentially lead to confusion.

Copy link
Contributor Author

@agocs agocs Feb 11, 2021

Choose a reason for hiding this comment

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

That's on the table for future improvements, but it's difficult to implement right now. Much more so than in interpreted languages [1]. One strategy we're looking into is potentially porting this logic into dd-agent-java (since that is already doing a lot of reflective stuff to decorate other classes), but we have some concerns about being beholden to dd-agent-java's release cycle. TBD.

[1]and, weirdly, the way Go has implemented reflection makes it a lot easier to wrap a Go handler than a Java handler.

Choose a reason for hiding this comment

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

I do agree that ideally we would have an @decorator added to a handler function, but I think this compromise is okay for now.

Copy link

@DarcyRaynerDD DarcyRaynerDD left a comment

Choose a reason for hiding this comment

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

LGTM on the whole Chris.

import io.opentracing.Scope;
import io.opentracing.SpanContext;
import io.opentracing.Tracer;
import io.opentracing.Tracer.SpanBuilder;

Choose a reason for hiding this comment

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

If a user hasn't installed the agent, do the open tracing libs have any effect on performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect the difference is sub-millisecond, but I'll verify that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, confirmed. The difference between the two isn't significant.

/**
* Finish the active span. If you have installed the dd-trace-java Lambda
* layer, you MUST call DDLambda.finish() at the end of your Handler
* in order to finish spans.

Choose a reason for hiding this comment

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

I do agree that ideally we would have an @decorator added to a handler function, but I think this compromise is okay for now.

@agocs agocs merged commit ad3a5f5 into main Feb 11, 2021
@agocs agocs deleted the chris.agocs/fix-distributed-tracing branch February 11, 2021 20:06
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