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/trace: only tag fully-covered spans as execution traced #1943

Merged
merged 4 commits into from
May 10, 2023

Conversation

nsrip-dd
Copy link
Contributor

@nsrip-dd nsrip-dd commented Apr 26, 2023

What does this PR do?

Only label spans as being covered by execution tracing if they are fully
covered, i.e. they start and finish with execution tracing enabled.
If we know that a span was partially covered, we can still label it, but
with something different so it's clear there is missing information.

Motivation

Currently we can see trace tasks which cover a shorter duration than the actual
span time, which is confusing.

Describe how to test/QA your changes

This PR includes a unit test.

Reviewer's Checklist

  • Changed code has unit tests for its functionality.
  • If this interacts with the agent in a new way, a system test has been added.

Sorry, something went wrong.

Only label spans as being covered by execution tracing if they are fully
covered, i.e. they start and finish with execution tracing enabled.
Otherwise, we can see trace tasks which cover a shorter duration than
the actual span time, which is confusing.
@nsrip-dd nsrip-dd force-pushed the nick.ripley/only-mark-completely-traced-spans branch from cbc5e64 to 1bd6889 Compare April 26, 2023 19:06
@pr-commenter
Copy link

pr-commenter bot commented Apr 26, 2023

Benchmarks

Comparing candidate commit 830380f in PR branch nick.ripley/only-mark-completely-traced-spans with baseline commit e4d62a7 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 0 unstable metrics.

@nsrip-dd nsrip-dd marked this pull request as ready for review May 2, 2023 15:05
@nsrip-dd nsrip-dd requested a review from a team May 2, 2023 15:05
felixge
felixge previously approved these changes May 8, 2023
Copy link
Member

@felixge felixge left a comment

Choose a reason for hiding this comment

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

LGTM. Did you consider implementing go_execution_traced:partial as well? Might be nice to still tag those spans that way?

If a span is paritally covered, a user might still be able to get some
use out of seeing the data. Give such spans an annotation, with a
different value than for fully-covered spans, so they can find the data.
I added a "partial" case to the existing execution trace span tagging
test, so we don't need a new test.
@nsrip-dd nsrip-dd merged commit d2e0f49 into main May 10, 2023
@nsrip-dd nsrip-dd deleted the nick.ripley/only-mark-completely-traced-spans branch May 10, 2023 17:13
katiehockman pushed a commit that referenced this pull request Jun 6, 2023
Only label spans as being covered by execution tracing if they are fully
covered, i.e. they start and finish with execution tracing enabled.
If we know that a span was partially covered, we can still label it, but
with something different so it's clear there is missing information.
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

3 participants