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

Support capturing stack trace #2163

Merged
merged 19 commits into from
Aug 13, 2021

Conversation

bhautikpip
Copy link
Contributor

@bhautikpip bhautikpip commented Aug 6, 2021

Add WithStackTrace option to add a stack trace when using span.RecordError or when panic is handled in span.End.

@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #2163 (914ddbc) into main (41588fe) will increase coverage by 0.0%.
The diff coverage is 83.8%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2163   +/-   ##
=====================================
  Coverage   72.7%   72.8%           
=====================================
  Files        177     177           
  Lines      12126   12151   +25     
=====================================
+ Hits        8825    8852   +27     
+ Misses      3062    3060    -2     
  Partials     239     239           
Impacted Files Coverage Δ
trace/config.go 63.7% <44.4%> (+6.4%) ⬆️
sdk/trace/span.go 90.0% <100.0%> (+0.4%) ⬆️

@bhautikpip
Copy link
Contributor Author

bhautikpip commented Aug 6, 2021

@Aneurysm9 @MrAlias Any pointers on how to expand this support for oteltest and opentracing package? Should we also implement the duplicate method of recordStackTrace in both of the packages?

sdk/trace/span.go Outdated Show resolved Hide resolved
sdk/trace/span.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
sdk/trace/span.go Outdated Show resolved Hide resolved
@jmacd
Copy link
Contributor

jmacd commented Aug 6, 2021

It strikes me that the decision to record a stack trace or not ought to be part of SDK configuration, not an instrumentation API. It's a lot like verbosity selection in the logging domain. It's on the same level as choosing your Histogram aggregator buckets in metrics, or sampler configuration to determine which spans record and which do not.

@tedsuo
Copy link

tedsuo commented Aug 6, 2021

@jmacd I think it might be both.

I can see why an end user would want a simple way to manually trigger a stack trace. The WithStackTrace API feels clean because it ensures the stack trace overhead is only triggered when the span is actually recording. That's better than the user having to manage the stack trace themselves.

At the same time, I agree with Josh that there probably needs to be a way to trigger stack trace collection from contrib/instrumentation packages, as there is no way for the end user/operator to control that behavior by writing code.

Like Josh mentioned, controlling these features feels kind of similar to the rules-based methods we've been discussing for controlling sampling and creating views - "when you see this pattern, adjust what you are collecting."

In general, it's hard to know in advance where you want to record a stack trace, so allowing an operator to control this type of behavior through a configuration language would probably be very valuable.

@bhautikpip
Copy link
Contributor Author

bhautikpip commented Aug 10, 2021

I have added WithStackTrace API as suggested by @MrAlias which can be used at end of the span or can be added in as an event option. We probably can migrate opt in support (for stack trace) to SDK configuration recommended by @jmacd in future probably once we have a path forward for sampling or other config mentioned in the comment above. However, I do not see that as a blocker to merge this PR.

sdk/trace/span.go Outdated Show resolved Hide resolved
trace/config.go Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
sdk/trace/span.go Outdated Show resolved Hide resolved
sdk/trace/span.go Outdated Show resolved Hide resolved
trace/config.go Outdated Show resolved Hide resolved
trace/config.go Outdated Show resolved Hide resolved
trace/config.go Show resolved Hide resolved
@bhautikpip
Copy link
Contributor Author

@Aneurysm9 @MrAlias @pellared addressed most comments! Have a look again. Thanks!

@pellared pellared changed the title capturing stack trace support Support capturing stack trace Aug 12, 2021
sdk/trace/span.go Outdated Show resolved Hide resolved
sdk/trace/trace_test.go Outdated Show resolved Hide resolved
sdk/trace/trace_test.go Outdated Show resolved Hide resolved
bhautikpip and others added 3 commits August 13, 2021 14:35
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@Aneurysm9 Aneurysm9 merged commit dfc866b into open-telemetry:main Aug 13, 2021
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

6 participants