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

ddtrace/tracer.StartSpan: Use setMeta instead of SetTag #1160

Merged
merged 5 commits into from Mar 8, 2022

Conversation

evanj
Copy link
Contributor

@evanj evanj commented Feb 7, 2022

The SetTag function takes an interface{}, so every string argument
must be converted using the convTString internal functions, which
allocates a wrapper. Since this is an internal function, we can use
setMeta directly. This seems to be what happens elsewhere in this
function. These are the only two uses of SetTag. This removes up to
2 memory allocations on each call to this function.

contrib/.../grpc: Add benchmark for env and version tags, and for the
NoDebugStack() option. This should more accurately represent how the
gRPC interceptor is typically configured. The benchstat output from
my laptop for 10 runs shows this change saves about 1-2% CPU, and
the expected 1 allocation for the env tag, because the version tag is
not set on the gRPC interceptor spans.

name                                                         old time/op    new time/op    delta
UnaryServerInterceptor/ok_no_metadata-8                        4.14µs ± 4%    4.11µs ±14%    ~     (p=0.190 n=10+10)
UnaryServerInterceptor/ok_with_metadata_no_parent-8            4.87µs ± 3%    4.77µs ± 3%  -1.96%  (p=0.011 n=9+9)
UnaryServerInterceptor/ok_with_metadata_with_parent-8          4.29µs ± 3%    4.18µs ± 1%  -2.50%  (p=0.000 n=10+9)
UnaryServerInterceptor/ok_no_metadata_with_analytics_rate-8    4.25µs ± 4%    4.13µs ± 1%  -2.76%  (p=0.002 n=10+9)
UnaryServerInterceptor/error_no_metadata-8                     10.6µs ± 3%    10.5µs ± 3%    ~     (p=0.113 n=10+9)
UnaryServerInterceptor/error_no_metadata_no_stack-8            5.24µs ± 2%    5.15µs ± 4%    ~     (p=0.051 n=10+9)

name                                                         old alloc/op   new alloc/op   delta
UnaryServerInterceptor/ok_no_metadata-8                        4.25kB ± 0%    4.23kB ± 0%  -0.38%  (p=0.001 n=10+10)
UnaryServerInterceptor/ok_with_metadata_no_parent-8            4.86kB ± 0%    4.84kB ± 1%  -0.42%  (p=0.001 n=7+10)
UnaryServerInterceptor/ok_with_metadata_with_parent-8          3.96kB ± 0%    3.95kB ± 0%  -0.28%  (p=0.017 n=10+10)
UnaryServerInterceptor/ok_no_metadata_with_analytics_rate-8    4.45kB ± 0%    4.44kB ± 0%    ~     (p=0.055 n=10+9)
UnaryServerInterceptor/error_no_metadata-8                     10.6kB ± 0%    10.6kB ± 0%  -0.32%  (p=0.001 n=10+9)
UnaryServerInterceptor/error_no_metadata_no_stack-8            5.56kB ± 0%    5.54kB ± 1%  -0.41%  (p=0.007 n=10+10)

name                                                         old allocs/op  new allocs/op  delta
UnaryServerInterceptor/ok_no_metadata-8                          43.0 ± 0%      42.0 ± 0%  -2.33%  (p=0.000 n=10+10)
UnaryServerInterceptor/ok_with_metadata_no_parent-8              48.0 ± 0%      47.0 ± 0%  -2.08%  (p=0.000 n=10+10)
UnaryServerInterceptor/ok_with_metadata_with_parent-8            39.0 ± 0%      38.0 ± 0%  -2.56%  (p=0.000 n=10+10)
UnaryServerInterceptor/ok_no_metadata_with_analytics_rate-8      45.0 ± 0%      44.0 ± 0%  -2.22%  (p=0.000 n=10+10)
UnaryServerInterceptor/error_no_metadata-8                       60.0 ± 0%      59.0 ± 0%  -1.67%  (p=0.000 n=10+10)
UnaryServerInterceptor/error_no_metadata_no_stack-8              47.0 ± 0%      46.0 ± 0%  -2.13%  (p=0.000 n=10+10)

The SetTag function takes an interface{}, so every string argument
must be converted using the convTString internal functions, which
allocates a wrapper. Since this is an internal function, we can use
setMeta directly. This seems to be what happens elsewhere in this
function. These are the only two uses of SetTag. This removes up to
2 memory allocations on each call to this function.

contrib/.../grpc: Add benchmark for env and version tags, and for the
NoDebugStack() option. This should more accurately represent how the
gRPC interceptor is typically configured. The benchstat output from
my laptop for 10 runs shows this change saves about 1-2% CPU, and
the expected 1 allocation for the env tag, because the version tag is
not set on the gRPC interceptor spans.

name                                                         old time/op    new time/op    delta
UnaryServerInterceptor/ok_no_metadata-8                        4.14µs ± 4%    4.11µs ±14%    ~     (p=0.190 n=10+10)
UnaryServerInterceptor/ok_with_metadata_no_parent-8            4.87µs ± 3%    4.77µs ± 3%  -1.96%  (p=0.011 n=9+9)
UnaryServerInterceptor/ok_with_metadata_with_parent-8          4.29µs ± 3%    4.18µs ± 1%  -2.50%  (p=0.000 n=10+9)
UnaryServerInterceptor/ok_no_metadata_with_analytics_rate-8    4.25µs ± 4%    4.13µs ± 1%  -2.76%  (p=0.002 n=10+9)
UnaryServerInterceptor/error_no_metadata-8                     10.6µs ± 3%    10.5µs ± 3%    ~     (p=0.113 n=10+9)
UnaryServerInterceptor/error_no_metadata_no_stack-8            5.24µs ± 2%    5.15µs ± 4%    ~     (p=0.051 n=10+9)

name                                                         old alloc/op   new alloc/op   delta
UnaryServerInterceptor/ok_no_metadata-8                        4.25kB ± 0%    4.23kB ± 0%  -0.38%  (p=0.001 n=10+10)
UnaryServerInterceptor/ok_with_metadata_no_parent-8            4.86kB ± 0%    4.84kB ± 1%  -0.42%  (p=0.001 n=7+10)
UnaryServerInterceptor/ok_with_metadata_with_parent-8          3.96kB ± 0%    3.95kB ± 0%  -0.28%  (p=0.017 n=10+10)
UnaryServerInterceptor/ok_no_metadata_with_analytics_rate-8    4.45kB ± 0%    4.44kB ± 0%    ~     (p=0.055 n=10+9)
UnaryServerInterceptor/error_no_metadata-8                     10.6kB ± 0%    10.6kB ± 0%  -0.32%  (p=0.001 n=10+9)
UnaryServerInterceptor/error_no_metadata_no_stack-8            5.56kB ± 0%    5.54kB ± 1%  -0.41%  (p=0.007 n=10+10)

name                                                         old allocs/op  new allocs/op  delta
UnaryServerInterceptor/ok_no_metadata-8                          43.0 ± 0%      42.0 ± 0%  -2.33%  (p=0.000 n=10+10)
UnaryServerInterceptor/ok_with_metadata_no_parent-8              48.0 ± 0%      47.0 ± 0%  -2.08%  (p=0.000 n=10+10)
UnaryServerInterceptor/ok_with_metadata_with_parent-8            39.0 ± 0%      38.0 ± 0%  -2.56%  (p=0.000 n=10+10)
UnaryServerInterceptor/ok_no_metadata_with_analytics_rate-8      45.0 ± 0%      44.0 ± 0%  -2.22%  (p=0.000 n=10+10)
UnaryServerInterceptor/error_no_metadata-8                       60.0 ± 0%      59.0 ± 0%  -1.67%  (p=0.000 n=10+10)
UnaryServerInterceptor/error_no_metadata_no_stack-8              47.0 ± 0%      46.0 ± 0%  -2.13%  (p=0.000 n=10+10)
@evanj evanj requested a review from a team February 7, 2022 14:29
@evanj
Copy link
Contributor Author

evanj commented Feb 7, 2022

We might want to merge PR #1135 first, because it adds a test that covers this code.

@felixge
Copy link
Member

felixge commented Feb 10, 2022

I merged #1135 and updated this PR. So this should be ready to review for @DataDog/tracing-go .

Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

This is a great catch 👍

@Julio-Guerra Julio-Guerra merged commit c47da55 into v1 Mar 8, 2022
@Julio-Guerra Julio-Guerra deleted the evan.jones/startspan-setmeta branch March 8, 2022 10:58
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

4 participants