Apm: Harmonize on TRANSACTION_MAX_SPANS

[beniwohli]

Description of the issue

Similar to #144 (regarding IGNORE_URLS), there's some variation how TRANSACTION_MAX_SPANS is implemented, specifically in how 0, -1 and larger negative values are interpreted:

1. Ruby, Python, Java: value is interpreted as number, no specific meaning is assigned to 0 or -1, meaning, any number <= 0 leads to all spans being dropped.
2. Node.js, .net: -1 is interpreted as "don't drop any spans". Larger negative values are invalid. .net falls back to default value, Node.js undefined behaviour.
3. Go: Only values > 0 are valid. For values <= 0, the feature is turned off, meaning no spans are dropped.

(The setting doesn't apply to RUM)

What we are voting on

Change the Go agent so it treats 0 as meaning "drop all spans", and in the ACM UI we will disallow setting negative values. No changes to agents to be consistent for values < 0; we might consider aligning on that later.

Vote

| Agent | Yes | No | Indifferent | N/A | Link to agent issue
| --------|:----:|:---:|:-----------:|:----:|:-------------------:|
| .NET |

• [x]

|

• [ ]

|

• [ ]

|

• [ ]

|
| Go |

• [x]

|

• [ ]

|

• [ ]

|

• [ ]

| https://github.com/elastic/apm-agent-go/issues/639
| Java |

• [x]

|

• [ ]

|

• [ ]

|

• [ ]

|
| Node.js |

• [x]

|

• [ ]

|

• [ ]

|

• [ ]

|
| Python |

• [x]

|

• [ ]

|

• [ ]

|

• [ ]

|
| Ruby |

• [x]

|

• [ ]

|

• [ ]

|

• [ ]

|
| RUM |

• [ ]

|

• [ ]

|

• [ ]

|

• [x]

|

Pinging @elastic/apm-agent-devs. There is some urgency here, as we have committed to support this TRANSACTION_MAX_SPANS in 7.5.

[beniwohli]

I'll start: I'm in favor of Option 1, mostly to keep it simple. Also, turning off the maximum span limit should be difficult, as there are very good reasons to have a limit in the first place:

• too many spans will make the UI unusable
• at some point, the overhead of collecting spans will start dominating the overall transaction time
• things like unbounded recursions can generate _a lot_ of spans, so having some escape hatch is almost always needed.

If a user absolutely wants to disable the limit, they can set it to a very large value. But I don't think we should make it easy or even document how to do that.

[watson]

I guess this is probably slightly different from language to language, but in Node.js it doesn't make it complicated to support the special meaning of -1. When we parse the config option when the process boots we simply replace -1 with the special number Infinity, which means that the actual logic for checking if the number of collected spans is over a certain threshold doesn't change with or without this behavior - it's simply: currentCount >= maxCount.

So I don't really buy into the "let's keep it simple" argument.

The argument that's left is whether or not we should allow users to shoot themselves in the foot by disabling this feature. And if it's as simple to implement as this is, I really don't see why not. We have learned time and time again that whatever we thought users would uses APM for, they will find a use-case we haven't considered.

But it's not a hill I'm willing to die on, so if I'm the only one with this view, let's just change the behavior of the Node.js agent.

[beniwohli]

"Keep it simple" for the user, not necessarily for the agent dev. Having special values that mean something else than what they are adds cognitive overhead and potentially surprising behavior.

[axw]

The Go agent can be configured using an options struct, like this:

tracer, err := apm.NewTracerOptions(apm.TracerOptions{...})

It's not currently possible to configure this particular setting via that struct, but eventually I would like it to be. In that case, you would do configure it like this:

tracer, err := apm.NewTracerOptions(apm.TracerOptions{
    TransactionMaxSpans: 99,
})

The problem with this is that if you don't explicitly specify TransactionMaxSpans, then the field takes on the zero value (literally, 0, since it would be an integer type). If we had 0 mean "drop all spans", then that wouldn't be a very friendly default :)

There are ways around this, such as making it a pointer field (so the zero value is nil, which we can interpret as meaning "use the default value"), and providing a function to return a pointer to an integer... I'd like to avoid this if at all possible, as it makes the API a bit clunky. OTOH, it's not going to be a common code path.

All of that said,

• I do think it's useful to be able to disable spans entirely
• This issue isn't limited to TRANSACTION_MAX_SPANS -- we have similar behaviour in STACK_TRACE_LIMIT and SPAN_FRAMES_MIN_DURATION

[beniwohli]

@axw pardon my ignorance, but why does not specifying TransactionMaxSpans not end up using the default value, which IIRC we agreed on 500 some time ago?

[axw]

@beniwohli to be clear, the TransactionMaxSpans I refer to above doesn't exist at the moment; I was trying to look forward to how this would impact the ability for users to specify configuration in code in an idiomatic manner. At the moment, you can only configure the Go agent with config either taken from the environment, or with default values; and then you can later change it if you want to.

However, what I had been imagining would only work if we took 0 to mean "use the default", i.e. 500. That's maybe a bit too confusing, so I'd probably end up going with the other approach I mentioned anyway (pointer value, nil = default).

[felixbarny]

we have similar behaviour in STACK_TRACE_LIMIT and SPAN_FRAMES_MIN_DURATION

@beniwohli would you suggest making all three behaving the same? I guess that would make sense for consistency's sake. In the Java agent, only those two have special meaning for -1, whereas TRANSACTION_MAX_SPANS has not. Or are you arguing that TRANSACTION_MAX_SPANS is special because spans should almost always be limited and therefore we should make it harder to make them unlimited (setting a very high number vs setting -1)?

[beniwohli]

@felixbarny it depends a bit what our goal is. My intention with opening this ticket was to find the shortest route to make TRANSACTION_MAX_SPANS a viable config option for remote configuration, so we can support it in 7.5. Hence my inclination with following the majority (AFAICT, this would only be a breaking change for the Node.js and Golang agents) (in addition to the arguments I made above).

If the goal is to go over _all_ config options and align all of them, then sure, let's also have a look at SPAN_FRAMES_MIN_DURATION etc. But that would involve a lot of discussion, specification work, and breaking changes, which I guess is not doable in a 7.5 time frame.

One doesn't preclude the other, so I definitely think we should go through _all_ shared config options and nail down how exactly they should be interpreteded by all agents. But I suggest that we do that in a different issue.

If there is one lesson out of all of this, is that we haven't been very careful with specifying config options as we added them, and that we need to do a better job at that with new config options we introduce.

[axw]

How about:

1. Change the Go agent so it treats 0 as meaning "drop all spans", and in the ACM UI disallow setting negative values. No changes to agents to be consistent for values < 0.

That would unblock us for now, and we can defer accepting negative values in the UI until later, if/when we make the agents consistent in their behaviour.

[axw]

No dissent, Go changes are done - closing this now.