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: create debug mode for old, open spans #2149

Merged
merged 35 commits into from
Aug 18, 2023

Conversation

hannahkm
Copy link
Contributor

@hannahkm hannahkm commented Jul 25, 2023

What does this PR do?

This PR introduces a debugging mode that the user can configure for finding and logging old, unfinished spans. Traces/Spans older than some configured X amount of time will be printed to the debug logs. When the tracer stops, it will also print all remaining open spans. For example, the debug mode can output log lines like the following:

2023/08/14 16:55:45 Datadog Tracer v1.55.0-dev WARN: 4 abandoned spans:
2023/08/14 17:15:38 Datadog Tracer v1.55.0-dev WARN: [name: newSpan, span_id: 7578134955455927291, trace_id: 7578134955455927291, age: 84 sec],[name: http.request, span_id: 5293323209049443106, trace_id: 5293323209049443106, age: 84 sec],[name: newSpan, span_id: 284037345165039393, trace_id: 284037345165039393, age: 84 sec],[name: http.request, span_id: 1770903217116959580, trace_id: 1770903217116959580, age: 84 sec],

Motivation

Traces with unfinished spans may never finish but never error, which causes confusion when data goes missing.

Fixes #2064

Describe how to test/QA your changes

New and old unit tests should pass. For manual testing, turn on the debug mode with WithDebugSpansMode(true) or by setting enviornment variable DD_TRACE_DEBUG_OPEN_SPANS=true. Create spans older than the configured amount of time (default value: 1 second, configure using environment variable DD_TRACE_OPEN_SPAN_TIMEOUT) and check that they are printed out in the tracer logs.

Call defer tracer.PrintOpenSpans() to check that a list of open spans is logged when the application ends.

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.

ddtrace/tracer/open_spans_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/open_spans_test.go Outdated Show resolved Hide resolved
note: tests are very slow. future commit hopefully will fix
@pr-commenter
Copy link

pr-commenter bot commented Jul 28, 2023

Benchmarks

Benchmark execution time: 2023-08-18 16:32:29

Comparing candidate commit 7c0f935 in PR branch hannahkm/debug-open-spans with baseline commit e9060fc in branch main.

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

ddtrace/tracer/open_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/open_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/open_spans.go Outdated Show resolved Hide resolved
@hannahkm hannahkm changed the title [WIP] ddtrace/tracer: create debug mode for old, open spans ddtrace/tracer: create debug mode for old, open spans Aug 4, 2023
@hannahkm hannahkm marked this pull request as ready for review August 4, 2023 19:21
@hannahkm hannahkm requested a review from a team August 4, 2023 19:21
Copy link
Contributor

@katiehockman katiehockman left a comment

Choose a reason for hiding this comment

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

Thanks Hannah! Some first comments. I haven't finished reviewing it yet, but wanted to give a chance to look over these first.
Mostly, I'd like to understand the context around the data structure decision. Why a linked list of generic linked lists, instead of something similar (e.g. a slice of span pointers)?

ddtrace/tracer/option.go Outdated Show resolved Hide resolved
ddtrace/tracer/option.go Outdated Show resolved Hide resolved
ddtrace/tracer/option.go Outdated Show resolved Hide resolved
ddtrace/tracer/option.go Outdated Show resolved Hide resolved
ddtrace/tracer/tracer.go Outdated Show resolved Hide resolved
ddtrace/tracer/open_spans.go Outdated Show resolved Hide resolved
@hannahkm
Copy link
Contributor Author

hannahkm commented Aug 7, 2023

Mostly, I'd like to understand the context around the data structure decision. Why a linked list of generic linked lists, instead of something similar (e.g. a slice of span pointers)?

Re the above: The main concern I had with creating the new data structure in the tracer was the overhead we'd introduce from adding/removing spans as they are started/finished. A linked list is great for this purpose (especially when removing spans from the middle of the structure), and the outer linked list is for the purposes of bucketing spans together. Spans that have a similar start time are placed in the same "bucket" so the new goroutine hopefully has less to scan through when checking for abandoned spans!

ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

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

Looks pretty good! A few questions / changes!

ddtrace/tracer/option.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans.go Show resolved Hide resolved
ddtrace/tracer/abandoned_spans_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/abandoned_spans_test.go Outdated Show resolved Hide resolved
ddtrace/tracer/tracer.go Outdated Show resolved Hide resolved
ddtrace/tracer/tracer.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

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

Thanks for being on the team!

@ajgajg1134 ajgajg1134 merged commit 283c0ac into main Aug 18, 2023
51 of 52 checks passed
@ajgajg1134 ajgajg1134 deleted the hannahkm/debug-open-spans branch August 18, 2023 20:53
devillecodes pushed a commit to agilebits/dd-trace-go that referenced this pull request Aug 18, 2023
ajgajg1134 added a commit that referenced this pull request Aug 21, 2023
@darccio darccio restored the hannahkm/debug-open-spans branch August 25, 2023 13:03
@darccio darccio deleted the hannahkm/debug-open-spans branch August 25, 2023 13: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.

proposal: tracer: Warn when developer code does not close all spans
4 participants