feat: Add WithStackTrace option for tracer#8094
feat: Add WithStackTrace option for tracer#8094AkselAllas wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
AkselAllas
requested review from
MrAlias,
XSAM,
dashpole,
dmathieu,
flc1125 and
pellared
as code owners
AkselAllas
force-pushed
the
feat/add-withstacktrace-option-to-tracer
branch
from
39c68b1 to
9580f43
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8094 +/- ##
=======================================
- Coverage 81.7% 81.7% -0.1%
=======================================
Files 308 308
Lines 23655 23684 +29
=======================================
+ Hits 19342 19358 +16
- Misses 3930 3943 +13
Partials 383 383
🚀 New features to boost your workflow:
|
dashpole
left a comment
dashpole
left a comment
There was a problem hiding this comment.
Should we propose this at the spec level?
| }) | ||
| } | ||
|
|
||
| // WithStackTrace configures the TracerProvider to capture a stack trace |
There was a problem hiding this comment.
Maybe something like WithAlwaysStackTrace() or something could make it clearer what this does.
|
|
||
| // WithStackTrace configures the TracerProvider to capture a stack trace | ||
| // for all recorded errors and panics. | ||
| func WithStackTrace(b bool) TracerProviderOption { |
There was a problem hiding this comment.
Thinking out loud: Do we want to be able to support anything else related to this? NeverStackTrace?
There was a problem hiding this comment.
I don't see the benefit of NeverStackTrace.
This would not allow the user to set the stackTrace on some important code path 🤔
There was a problem hiding this comment.
Some possible use-cases:
- Your backend doesn't support stack traces
- An instrumentation library you don't control is collecting too many stack traces and you want to disable it.
Perhaps. I know we will need something like this for Typescript and C# as well as we will want Datadog Error Tracking to work for that as well. |
|
@dashpole Can we ignore these 2 uncovered test lines? 
These were uncovered before as well 🤔 |
| ) | ||
| } | ||
|
|
||
| func TestProviderRecordErrorWithStackTrace(t *testing.T) { |
There was a problem hiding this comment.
It looks like this and the other test are mostly the same as the tests above them. If it makes sense, make these table-driven instead of separate tests.
|
Yes, don't worry about lines that weren't previously covered. |
|
@dashpole Can you re-approve CICD? |
|
@dashpole Fixed lint. Need CICD reapprove. |
AkselAllas
force-pushed
the
feat/add-withstacktrace-option-to-tracer
branch
from
891812c to
e35fbc6
Compare
|
@dashpole Sorry for lint issues. Was using wrong local golingci-lint instead of |
|
@dashpole Seems ready for hopefully final review. |
|
@dashpole What's the next step with this PR? |
|
Everyone is at kubecon EU, so getting feedback from others will be slow. Given you intend to add this across languages, I would like to see this added to the specification (for agreement on naming and structure), stabilized, and added to the declarative configuration format before we add it to the public API for this repo. If you need something sooner, we should change this PR to not make any public api changes, and use "feature" environment variables to enable the behavior (search the repository for "x.go" for examples). |
|
@dashpole Thanks for the response! I personally don't have a plan to implement this in all the languages. I fully prefer getting this out asap, so I will go with the feature env vars approach 🙇 |
|
If it is implemented in more than one language, it is good to add it to the spec. Note that the env vars approach is designed for features that are experimental in the specification. If we add this to this repo as experimental, and there isn't interest in adding it to the specification and stabilizing it, then it will likely be removed in a future version. |
|
On second thought, I think I would prefer to take the spec route. Who submits this to spec? And how does the process look like? |
|
Anyone can submit changes to the spec. Start with an issue in opentelemetry-specification describing your use-case. Propose a high level solution (what you would change in the spec) and list some alternatives you considered. The issue will get assigned a spec sponsor who can help you progress. Open a PR with the proposed spec change, and point to this PR (in otel-go) as your prototype. Make sure it is marked as in development, and has a row in the spec compliance matrix. It can help to raise it at the Specification SIG meeting, but that isn't strictly necessary. You just need two approvals on the spec PR for it to merge. After that, we need experimental implementations in 3 languages, and wait for feedback. Then we stabilize the spec, and try to implement it everywhere. |
|
Thanks for the response! |
There was a problem hiding this comment.
We are planning to deprecate Span.RecordError. See: open-telemetry/opentelemetry-specification#4825 and https://opentelemetry.io/blog/2026/deprecating-span-events/.
|
@pellared Even with deprecation, the tracing events api will be usable for multiple years I assume? I would be fine with being able to use this feature for that amount of time. |
|
@AkselAllas, we are not removing the Span Events API. However, since we are actively deprecating this area, it’s unlikely that OTel maintainers will support introducing new features related to it. |
I still think it's a useful and simple feature to have. I am still in favor of continuing with this PR. |
3 participants
For me use case is that in order to be able to use error tracking software based off stackTraces, I want to be able to set stackTraces for all spans via a shared library, which does not need an additional callsite via e.g. http instrumentation via opentelemetry-go-contrib.
Extra context from here: https://cloud-native.slack.com/archives/C01NPAXACKT/p1765159082954469