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

span: Add config setting exit_span_min_duration #1138

Merged
merged 14 commits into from
Oct 27, 2021
Merged

span: Add config setting exit_span_min_duration #1138

merged 14 commits into from
Oct 27, 2021

Conversation

marclop
Copy link
Contributor

@marclop marclop commented Oct 14, 2021

Description

Adds a new configuration setting which drops discardable exit spans when
their duration is lower or equal than exit_span_min_duration. If span
compression is enabled, the duration of the composite is considered.

This setting drops the spans after they have ended in contrast with the
existing transaction_max_spans which drops them from the start.

The existing a new configutil.ParseDurationOptions has been introduced
which allows specifying a custom unit as the minimum duration.

Updates the end() private method and moves the compression attempts to
the Public function so that every iteration of cachedSpan.end() does
not have to go through that logic again.

Moves the s.dropped logic to the private end function since it now
needs to check if the span is discardable on every span end, but only
after the compression cache has been evicted.

Most of the lock acquisition has been moved out of the end() function
and onto the public End(). The main reason is because the transaction
may call cachedSpan.End() with a different set of locks acquired vs
span.End().

Last, fixes a small bug where transaction.dropped_spans_stats.outcome
was empty if the outcome hadn't been set in the modelwriter.

Related issues

Closes #1112

Adds a new configuration setting which drops discardable exit spans when
their duration is lower or equal than `exit_span_min_duration`. If span
compression is enabled, the duration of the composite is considered.

This setting drops the spans after they have ended in contrast with the
existing `transaction_max_spans` which drops them from the start.

The existing a new `configutil.ParseDurationOptions` has been introduced
which allows specifying a custom unit as the minimum duration.

Signed-off-by: Marc Lopez Rubio <[email protected]>
Updates the `end()` private method and moves the compression attempts to
the Public function so that every iteration of `cachedSpan.end()` does
not have to go through that logic again.

Moves the `s.dropped` logic to the private `end` function since it now
needs to check if the span is discardable on every span end, but only
after the compression cache has been evicted.

Most of the lock acquisition has been moved out of the `end()` function
and onto the public `End()`. The main reason is because the transaction
may call `cachedSpan.End()` with a different set of locks acquired vs
`span.End()`.

Last, fixes a small bug where `transaction.dropped_spans_stats.outcome`
was empty if the outcome hadn't been set in the modelwriter.

Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
@marclop marclop added enhancement New feature or request v7.16.0 labels Oct 14, 2021
@marclop marclop self-assigned this Oct 14, 2021
Signed-off-by: Marc Lopez Rubio <[email protected]>
@apmmachine
Copy link
Contributor

apmmachine commented Oct 14, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-10-27T00:41:51.407+0000

  • Duration: 33 min 26 sec

  • Commit: 63f78c7

Test stats 🧪

Test Results
Failed 0
Passed 11564
Skipped 268
Total 11832

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

Signed-off-by: Marc Lopez Rubio <[email protected]>
span.go Outdated Show resolved Hide resolved
Signed-off-by: Marc Lopez Rubio <[email protected]>
docs/configuration.asciidoc Outdated Show resolved Hide resolved
docs/configuration.asciidoc Outdated Show resolved Hide resolved
tracer.go Outdated Show resolved Hide resolved
internal/configutil/duration.go Outdated Show resolved Hide resolved
module/apmgoredisv8/hook_test.go Outdated Show resolved Hide resolved
transaction_test.go Outdated Show resolved Hide resolved
span_test.go Outdated Show resolved Hide resolved
span.go Outdated
s.setStacktrace(1)
}

if s.tx != nil {
s.reportSelfTime()
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about removing the locking of s.parent/s.tx from reportSelfTime and attemptCompress, and adding those locks toSpan.End? Then we can move this reportSelfTime call to the s.tx != nil block below.

span.go Show resolved Hide resolved
span.go Outdated Show resolved Hide resolved
@marclop
Copy link
Contributor Author

marclop commented Oct 26, 2021

Seems like versions <= go 1.10 started failing because of the github.com/mattn/go-sqlite3 dependency.

go get -v -t ./...
github.com/mattn/go-sqlite3
# github.com/mattn/go-sqlite3
../../github.com/mattn/go-sqlite3/sqlite3_type.go:75: undefined: sql.NullTime

EDIT: This is the commit that seems to have broken the build: mattn/go-sqlite3@5671e01#diff-66d608cc54b91bcae2b1f650ee68243c099d7b81bcb7a6a88292a94798580185.

Signed-off-by: Marc Lopez Rubio <[email protected]>
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

Implementation looks good. I'm struggling a bit to follow the tests, but I probably just need some time to get used to them. One question and a couple of nits, otherwise LGTM

span.go Outdated Show resolved Hide resolved
span_test.go Outdated Show resolved Hide resolved
span_test.go Outdated Show resolved Hide resolved
span_test.go Outdated Show resolved Hide resolved
@marclop
Copy link
Contributor Author

marclop commented Oct 26, 2021

@axw Added a few comments on the tests and updated the transaction's counters:

  • tx.spansCreated will be decremented when a span is compressed into the cache and when a fast exit span is dropped.
  • tx.spansDropped will be incremented when a fast exit span is dropped.

With these changes, dropping fast exit spans will not decrement the spansCreated count so that it reflects the spans that will be reported to the apm-server and a compressed span will count as a single span.

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

LGTM, just one possible minor simplification.

span.go Outdated Show resolved Hide resolved
Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
@marclop marclop merged commit a8ec181 into elastic:master Oct 27, 2021
@marclop marclop deleted the f/add-exit_span_min_duration-drop-fast-exit-spans branch October 27, 2021 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[META 496] Dropping fast exit spans
3 participants