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

handle special cases for span frames min duration #1188

Conversation

stuartnelson3
Copy link
Contributor

@stuartnelson3 stuartnelson3 commented Jan 11, 2022

cases:

0: never capture stacktraces
-1: always capture stacktraces

Closes #465

cases:

0: never capture stacktraces
-1: always capture stacktraces
Comment on lines +359 to +369
switch s.stackFramesMinDuration {
case -1:
// Always set stacktrace
s.setStacktrace(1)
case 0:
// If s.stackFramesMinDuration == 0, we never set stacktrace.
default:
if !s.dropped() && len(s.stacktrace) == 0 &&
s.Duration >= s.stackFramesMinDuration {
s.setStacktrace(1)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could have been squished into a single conditional, and I'm happy to do so. This just seemed to be more easy to read.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me. Maybe we can revisit when we get to #1142

@apmmachine
Copy link
Contributor

apmmachine commented Jan 11, 2022

💚 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: 2022-01-12T07:57:14.389+0000

  • Duration: 32 min 19 sec

  • Commit: 411b6d1

Test stats 🧪

Test Results
Failed 0
Passed 12117
Skipped 279
Total 12396

🤖 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.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

span_test.go Outdated

spans = tracer.Payloads().Spans
require.Len(t, spans, 1)
assert.Len(t, spans[0].Stacktrace, 4)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just check it's non-empty? The Go runtime or testing package might add more frames in the future.

@stuartnelson3 stuartnelson3 enabled auto-merge (squash) January 12, 2022 07:57
@stuartnelson3 stuartnelson3 merged commit cfb2b83 into elastic:master Jan 12, 2022
@stuartnelson3 stuartnelson3 deleted the special-handling-span-frames-min-duration branch January 12, 2022 10:15
@simitt simitt added this to the 2.0 milestone Feb 9, 2022
@axw axw self-assigned this Feb 10, 2022
@axw
Copy link
Member

axw commented Feb 10, 2022

Verified using go.elastic.co/apm/v2 v2.0.0-20220209134054-640a916049c9 (current main):

package main

import (
        "time"

        "go.elastic.co/apm/v2"
)

func main() {
        tracer := apm.DefaultTracer()
        defer tracer.Flush(nil)

        tx := tracer.StartTransaction("name", "type")
        defer tx.End()

        span := tx.StartSpan("name", "type", nil)
        time.Sleep(5 * time.Millisecond)
        span.End()
}
  • By default, the span has a stack trace
  • When setting ELASTIC_APM_SPAN_FRAMES_MIN_DURATION=-1ms, the span still has a stack trace
  • When setting ELASTIC_APM_SPAN_FRAMES_MIN_DURATION=0ms, the span does not have a stack trace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add special case value to disable ELASTIC_APM_SPAN_FRAMES_MIN_DURATION
4 participants