Add option for limiting the number of logs in a Span

[RaduBerinde]

The MaxLogsPerSpan option limits the number of logs in a Span. Oldest events are dropped as necessary. The first log in the finished span indicates how many were dropped.

The default limit is 100.

Fixes #38.

CC @tschottdorf @bensigelman

[jmacd]

@yurishkuro If it's a tag, then we don't know where the dropped logs span begins and ends. ᐧ

On Mon, Sep 26, 2016 at 2:27 PM, Yuri Shkuro notifications@github.com wrote:

@yurishkuro commented on this pull request.

In span.go https://github.com/opentracing/basictracer-go/pull/39#pullrequestreview-1628204 :

• // buffer in place, moving the first pos elements to the back. This
• // algorithm is described in:
• // http://www.cplusplus.com/reference/algorithm/rotate
• for first, middle, next := 0, pos, pos; first != middle; {
• s.raw.Logs[first], s.raw.Logs[next] = s.raw.Logs[next], s.raw.Logs[first]
• first++
• next++
• if next == len(s.raw.Logs) {
• next = middle
• } else if first == middle {
• middle = next
• }
• }
• // Replace the oldest event. This means we are effectively dropping one more
• // event.
• s.raw.Logs[0].Event = fmt.Sprintf("\ dropped %d events **", s.numDroppedLogs+1)

perhaps this makes more sense as a tag than a log

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/opentracing/basictracer-go/pull/39#pullrequestreview-1628204, or mute the thread https://github.com/notifications/unsubscribe-auth/ADdiiRwl9YyR7uTxKIEzB9KpHeTIX6JVks5quDjGgaJpZM4KG8nA .

[RaduBerinde]

Also, in some cases tags might not be visible everywhere where logs are. A prime example is NetTraceIntegrator which ignores tags (though that should be fixed IMO).

[yurishkuro]

then we don't know where the dropped logs span begins and ends.

hm, if your limit is 10 and you dropped 50 logs, how exactly do you know where the dropped ones begin? My reasoning is that the number of dropped logs is an attribute of the whole span, attaching a timestamp to that information (via log) does not have a lot of meaning.

[RaduBerinde]

I think what was meant was that the message makes it obvious which logs were dropped in relation to the ones which were not dropped. In the current change, the ** dropped ** message comes first so it's clear the older logs were dropped. In the proposal above, it comes in the middle; we definitely need some kind of "separator" between the oldest and the newest logs in that case.

[jmacd]

I was thinking that a limit serves two purposes: (1) limit memory (2) limit bandwidth From the service's point of view (and network egress costs) it's useful to have a limit, and I would probably wish for it to be applied to bulk log data as well. Though I suppose if you're using bulk logs, you can limit it yourself.

As for the timestamp, my proposal was that by inserting a log record at the point were logs are dropped, at least the user can see two complete subsections of the log, the beginning up until the drop point, and the drop point to the end. I.e., there are two pieces of information, how many were dropped and which time range was dropped.

[RaduBerinde]

Sure, I will apply the limit to the bulk data as well.

Also, is the default limit of 100 ok? (I don't feel strongly about any particular value, I would just avoid setting it too small given that it will change the behavior of current users)

[RaduBerinde]

Are there any objections to @jmacd's proposal to drop the events in the "middle"? If not, I will work on that.

[bhs]

@RaduBerinde I think @jmacd's "middle-out" (har har har) proposal is the optimal thing if you can spare the time. We did something similar with Dapper's version of this change and it worked well.

[RaduBerinde]

Updated to "middle-out" :)

One thing I'm not sure is how to "format" the "dropped logs" log record (see span.go:214).

[RaduBerinde]

I see that Lightstep only shows the value of the event field. Should I include the number of dropped logs in the "event" string (at least for now)?

[bhs]

@RaduBerinde no, that is a lightstep limitation and it should be fixed on that side.

[bhs]

This LGTM... I'd like to wait for @yurishkuro and @jmacd to give a :thumbsup: before merging, though.

[bhs]

@RaduBerinde you'll need to rebase this one...

[RaduBerinde]

Updated. Thanks for the reviews!

[bhs]

LGTM. I'll give others until tomorrow to raise any other concerns before merging (esp @jmacd since @yurishkuro already approved the change). Thanks again!