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

contrib: Update non-client contrib packages to measure their spans #603

Merged
merged 18 commits into from Mar 13, 2020

Conversation

knusbaum
Copy link
Contributor

@knusbaum knusbaum commented Feb 27, 2020

Until recently, spans were measured only when they were considered "top-level" for a service - i.e. their service name differed from their parent's. Now we have support for manually marking a span to be measured (#591).

Since we have this option, we want to mark relevant spans in our integrations to be measured. Any span that sets its own service name, and is most likely to have a parent span from the user's code or a different integration should be measured. Any spans that will be children of spans from the same integration don't need to be measured.

There is another caveat, which is that we don't want to measure spans that are for integrations for external service clients. This is a very generic class of integrations, and includes any integration for a package that is primarily an API to an external service. This includes SQL integrations, the aws integration, the client portion of the grpc integration, the http client integration, etc. This is actually most of our integrations. The reason for this is that there can be conflicts when measuring client spans that may cause inaccurate metrics.

@knusbaum knusbaum added this to the 1.23.0 milestone Feb 27, 2020
@knusbaum knusbaum force-pushed the knusbaum/measured-span-integrations branch from a24ca16 to 27c7640 Compare February 27, 2020 16:02
@knusbaum knusbaum force-pushed the knusbaum/measured-span-integrations branch from 27c7640 to 2eb1c21 Compare February 27, 2020 16:11
contrib/julienschmidt/httprouter/httprouter.go Outdated Show resolved Hide resolved
contrib/gorilla/mux/mux.go Outdated Show resolved Hide resolved
@knusbaum knusbaum changed the base branch from apm/add-measured-span-api-to-go-tracer to v1 March 3, 2020 20:52
contrib/gin-gonic/gin/gintrace.go Outdated Show resolved Hide resolved
contrib/miekg/dns/dns.go Outdated Show resolved Hide resolved
@knusbaum knusbaum force-pushed the knusbaum/measured-span-integrations branch from 87fb359 to f814af8 Compare March 5, 2020 20:05
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Can you please set a proper title and a description to this PR? It doesn't look like "all contrib packages" to me.

contrib/google.golang.org/grpc/grpc.go Outdated Show resolved Hide resolved
@knusbaum knusbaum changed the title contrib: Update all contrib packages to measure their spans contrib: Update server contrib packages to measure their spans Mar 12, 2020
@knusbaum knusbaum changed the title contrib: Update server contrib packages to measure their spans contrib: Update non-client contrib packages to measure their spans Mar 12, 2020
@knusbaum knusbaum requested a review from gbbr March 13, 2020 00:36
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Looks mostly ok. How did you come up with the list of integrations to enable this for? I thought it's only for web frameworks but I also see Kafka.

contrib/google.golang.org/grpc/grpc.go Show resolved Hide resolved
ddtrace/tracer/option.go Outdated Show resolved Hide resolved
Co-Authored-By: Gabriel Aszalos <gabriel.aszalos@gmail.com>
@knusbaum
Copy link
Contributor Author

knusbaum commented Mar 13, 2020

How did you come up with the list of integrations to enable this for? I thought it's only for web frameworks but I also see Kafka.

The criteria I was given was "every integration that's not a client for an external service." I know the kafka producer is a network client, but that one was explicitly approved. They decided to do the producer and skip the consumer. The rest end up being mostly http servers and grpc.

@knusbaum knusbaum requested a review from gbbr March 13, 2020 14:25
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

Nice and clean! :) Thanks!

@knusbaum knusbaum merged commit 8fb554f into v1 Mar 13, 2020
@knusbaum knusbaum deleted the knusbaum/measured-span-integrations branch March 13, 2020 15:39
mingrammer pushed a commit to mingrammer/dd-trace-go that referenced this pull request Dec 22, 2020
…ataDog#603)

This commit marks relevant spans in our integrations to be measured. Previously, choosing which spans are measured was done elsewhere. We are able to do this now that we have support for manually marking spans to be measured (DataDog#591). 

A relevant span is any span that sets a service name, and is likely to have a parent span from the user's code or a different integration. Any spans that will be children of spans from the same integration don't need to be measured.

We also don't measure spans that are for integrations for external service clients. This is a very generic class of integrations, and includes any integration for a package that is primarily an API to an external service. This includes SQL integrations, the aws integration, the client portion of the grpc integration, the http client integration, etc. This is actually most of our integrations. The reason for this is that there can be conflicts when measuring client spans that may cause inaccurate metrics.
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.

None yet

5 participants