feat: use dd-trace-go v2 #206
Conversation
|
dd-trace-go v2.1.0 enables client-side stats computation by default. Can we turn it off by default? |
Updated accordingly. |
# Conflicts: # go.mod # go.sum # internal/extension/extension.go
joeyzhao2018
force-pushed
the
joey/ddtracegoV2
branch
from
eed3640 to
5eaac2d
Compare
|
@joeyzhao2018 i think we also need to disable telemetry by default too DataDog/dd-trace-go#3808 |
|
That would be fantastic @happynancee if we could also land that change 👍
|
I explicitly set DD_INSTRUMENTATION_TELEMETRY_ENABLED to false in the tracer initialization code block. |
Thanks @joeyzhao2018 - can see the change in 4d70949. 👍 |
| } else { | ||
| logger.Error(fmt.Errorf("could not get sampling priority from getSamplingPriority()")) | ||
| } | ||
| logger.Error(fmt.Errorf("could not get sampling priority from spanContext.SamplingPriority()")) |
There was a problem hiding this comment.
spanContext.SamplingPriority is directly available in dd-trace-go v2. No longer need the handling for the adapter case
purple4reina
left a comment
purple4reina
left a comment
There was a problem hiding this comment.
This all looks great!
One question. For a customer who is currently using the v1 tracer in their lambda code, how will upgrading to this new version affect them? Will their spans all of a sudden be orphaned? Will their code even compile?
This is an important question for go because our customers will rely heavily on adding their own custom instrumentation to their functions.
To clarify, this is indeed a breaking change as many of the APIs are different in dd-trace-go V2. We will need to bump the major version when releasing it next time and reference the migration guide. But it shouldn't cause any orphaned spans or any feature-wise issues.
Again, the customers do need to migrate and adapt to the new APIs. However, since we don't release go tracer as layers, releasing a datadog-lambda-go v2 doesn't block us to do any fix in v1 if needed. |
FYI, doing a major version bump in golang is VERY different from other languages. Read about it here. It requires copying your entire source into a new |
Thank you for sharing this! This is very helpful. After thinking more about it and some research, I think we actually have to stay as v1 following go's convention. Because there's no breaking change in the public apis for all the lambda wrappers. In other words, it will be backward compatible. What I said earlier was inaccurate by the As for migrating the dd-trace-go, we are still doing it and probably going to use Orchestrion as a few customers have already requested it. I don't think this PR is blocked by that migration though. |
|
Sadly, bumping to dd-trace-go v2 won't be backward compatible. Consider the following customer app where they are using dd-trace-go v1.73.1: package main
import (
"context"
"time"
ddlambda "github.com/DataDog/datadog-lambda-go"
"github.com/aws/aws-lambda-go/events"
"github.com/aws/aws-lambda-go/lambda"
"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer"
)
func main() {
lambda.Start(ddlambda.WrapFunction(myHandler, nil))
}
func myHandler(ctx context.Context, event events.APIGatewayProxyRequest) (string, error) {
span, ctx := tracer.StartSpanFromContext(ctx, "hello-rey")
defer span.Finish()
time.Sleep(time.Second)
return "ok", nil
}Using the current version of datadog-lambda-go, they will see a trace like this, with a single custom span: 
However, after updating to use this branch and without any code changes in the handler, there are now two tracers installed: v1 which the customer is using to create their custom span and v2 which is now being used by datadog-lambda-go. This change will lead to orphaned spans and a trace that looks like: 
Now, there is some nuance here, which you have already pointed out. If we instead were to create a new v2 of datadog-lambda-go, and the customer uses the same handler file as I shared above, they will still get the same result. You've also pointed out correctly that the API defined in datadog-lambda-go isn't changing with this PR. So why then should we do a v2 release? Because the changes proposed in this PR will (most likely) require customers to make code changes to their functions. If they are currently actively using v1 in their code, their traces will be broken if they upgrade to this branch. You're right. There isn't a breaking change to our api. Customer code will still compile. However, I believe it is best practice for us to introduce a v2 here. If customers want their functions to behave exactly as they did with dd-trace-go v1, then there are code changes they will have to make. If I have to update my code when upgrading a package in order to maintain current functionality, then that is by definition a breaking change which requires a major version bump. I do additionally think this major version change offers us an opportunity we should take. As we've talked about several times before, we'd like to eventually do away with datadog-lambda-go and migrate everything to dd-trace-go instead. This v2 version bump provides us with an opportunity to do that. |
|
So TL;DR @joeyzhao2018 we should expect to see this PR canned/closed and a new upcoming This is how I read DataDog/dd-trace-go#3943? |
|
Is there an update on this? We have a medium CVE that we'd like to get rid of (CVE-2020-8911) but we cannot until this library uses the tracer v2 library. |
|
@epot Sorry for the late response. I'm going to close this PR as we have migrated the datadog-lambda-go to dd-trace-go DataDog/dd-trace-go#3943. The next release of dd-trace-go will contain the datadog-lambda-go module in it. |
|
@joeyzhao2018 I am sorry but I don't fully get it.
|
|
Hi @epot, I'm afraid that might be the case for the vulnerability scanners? To be honest, I'm actually not very familiar with the vulnerability scanners. The link you attached directs me to aws-sdk-go v1.34.0 but I'm not finding that dependency in dd-trace-go v1.74.6 or datadog-lambda-go v1.29.0. So I think I need some assistance to understand it better so that we can release a version without that problematic dependency. Thank you. |
|
I am not 100% sure, but if you look here https://github.com/DataDog/dd-trace-go/blob/v1.74.6/go.mod#L69 then you can see a dependency to aws SDK v1, which contains the medium vulnerability I linked. Assuming I understood correctly and my projet will still depend on |
|
Sorry @joeyzhao2018 - but the way this has been handled from Datadog really feels sub-par for paying customers of your product. What's the plans for now?
The communication around this could and should be much better. As of right now, we're still on the Do I need to raise all these concerns via our account manager? |
|
Hi @magnetikonline, datadog-lambda-go is not deprecated yet. We will start the deprecation process once the next dd-trace-go releases with datadog-lambda-go in it. The migration is a direct code base migration without any code logic changes and also included all the unittests and integration tests, therefore it is expected to work. In fact, for customers already using the latest datadog-lambda-go (v1.29.0 as of now), it's guaranteed to work because the underlying dd-trace-go used in v1.74.6+, which actually uses dd-trace-go v2 via an adapter. Regarding the noisy instrumentation telemetry logs, I recommend setting DD_INSTRUMENTATION_TELEMETRY_ENABLED manually as a workaround for now. Sorry for the wait but all this effort will make sure our future updates more smooth and more frequent. |
|
Hi @epot, sorry about the situation. Unfortunately, I think the only workaround is to manually change to use [email protected] (latest).
We actually finished migration to dd-trace-go. In the next release of dd-trace-go, it would contain the module Let me know if you have any other questions. |
|
Do you know when the release is due? Thanks!! |
|
Hi @epot, we are actually in the process of releasing. I expect it be ready within this month. |
|
Hi @epot and all, dd-trace-go v2.4.0 got released, and you can use |
|
Great! Thank you! |
What does this PR do?
Motivation
#204
Implementation Details
Span.StartChild. In "Serverless Universal Instrumentation", that span is created in the agent instead of the tracer.Testing Guidelines
Additional Notes
Types of changes
Checklist