Signify that a stack trace was truncated

mikker commented 5 years ago

Description of the issue

A few agents (Java, Ruby at least) have the option to truncate stack traces to x lines. Currently this is done by simply chopping off the bottom.

However there's no indication in the UI to the user that this has happened, perhaps leading to confusion.

Suggestions for implementation:

stacktrace: { truncated: true }. A simple boolean is likely the simplest solution.
stacktrace: { truncated: 9 }. However a count is probably just as easy to save while giving a bit more information.
stacktrace: { truncated: [[0, 3], [6, 10]] }. If we want to get even fancier in the future, for example doing middle truncation or truncation depending on lines being library or application code, we need to store the exact indexes so the UI can show ...truncated... in the right places.

UPDATE

We go with the simplest implementation, stacktrace: { truncated: true }.

I suggest we make it optional on the server side for compatibility with older agents but default to false in agents when implemented?

What we are voting on

Whether this is a feasible (and good enough) solution. Added a row for APM Server as this is going into the api too.

Vote

Agent	Yes	No	Indifferent	N/A
APM Server	[ ]	[ ]	[ ]	[ ]
.NET	[x]	[ ]	[ ]	[ ]
Go	[ ]	[ ]	[ ]	[ ]
Java	[x]	[ ]	[ ]	[ ]
Node.js	[ ]	[ ]	[ ]	[ ]
Python	[x]	[ ]	[ ]	[ ]
Ruby	[x]	[ ]	[ ]	[ ]
RUM	[ ]	[ ]	[ ]	[ ]

watson commented 5 years ago

In Node.js/V8 it's possible to set a global stack trace limit. This means that all errors generated in your application will have their stack trace limited to the last x frames. By default, this limit is 10 frames (when a user installs the Node.js agent we change this global limit to 50).

The benefit of a limit is that the JavaScript engine doesn't have to spend time and resources generating stack traces that have more frames than this.

Without setting this limit to Infinity, we can't, unfortunately, support the last option you suggested. But we can do either of the first two 🙂

Update: The config option used in the Node.js agent to set this is called stackTraceLimit

mikker commented 5 years ago

Got it. Depending on what's possible in the other langs (and what we decide to do) Node could just be the exception.

axw commented 5 years ago

The Go agent currently has no config for this, it just grabs the whole stack track. https://github.com/elastic/apm-agent-go/issues/526 is open to implement the config.

For Go, including truncated: true would be easy enough: just need to reserve space for one additional program counter beyond the specified limit. The other two options are more expensive, requiring an arbitrary number of allocations.

vigneshshanmugam commented 5 years ago

Whatever @watson mentioned applies to RUM agent as well plus different JS engines have different frame limits AFAIK. Without setting the stack-trace to Infinity, we cannot truncate at arbitrary lines.

mikker commented 5 years ago

Sounds to me like we should go with the simplest option then. Using different value types for different agents are probably both 1. error prone in UI and 2. adds overhead to the schema validation.

I'll update the description.

Qard commented 5 years ago

@watson We don't have a way of knowing if the stack depth exceeds the limit though, as it's applied internal to V8. We'd have to set it to Infinity and chop it ourselves. 🤔

watson commented 5 years ago

@Qard Good point. I expect that would be pretty wasteful though 🤔

mikker commented 5 years ago

IMO Node could just ignore the flag.

watson commented 5 years ago

Absolutely - As long as we just don't make the flag required. Then we could say that the absence of the flag meant that it was unknown

gregkalapos commented 4 years ago

We discussed this in today's agent planning.

Summary:

If it can't be implemented in Node.js or RUM, we could still have it for other agents
For .NET this is small effort to implement
This does not sound like high priority right now
@nehaduggal said this should bubble up to product roadmap discussion and be coordinated that way.
2 options seemed to be realistic:
- Open implementation issues for all agents except RUM and Node.js and implement it.
- Close it and decide to not do it.

Maybe one suggestion from my side: given this is more than a year old maybe @alex-fedotyev or @nehaduggal could lead this and own this issue, collect input and make a decision.

basepi commented 4 years ago

Python limits by frames, not lines. I do think this would be useful for the python agent.

elastic / apm