Skip to content
This repository has been archived by the owner on Oct 29, 2021. It is now read-only.

Improvements on Recorder. #135

Merged
merged 1 commit into from
Apr 16, 2016
Merged

Conversation

chris-ramon
Copy link
Contributor

@chris-ramon chris-ramon commented Apr 8, 2016

Details

Related issue: #132

  • Adds Recorder.Finish(...) method, such that a span will only be recorded to once (a recorder produces only one Store.Collect(...) call for a given SpanID).
  • Updates httptrace client & server to call Recorder.Finish(...) in order to collect the span.
  • Fixes recorder tests.

r.annotations = append(r.annotations, as...)
}

func (r *Recorder) Finish() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add:

// Finish finishes recording and saves the recorded information to the
// underlying collector. If Finish is not called, then no data will be written
// to the underlying collector.

@bg451
Copy link
Contributor

bg451 commented Apr 8, 2016

I'd vote for AggregateStore not being removed, since one of the nice things about Appdash is that it can be embedded in an application without many dependencies, ie influxdb in this case.

@chris-ramon
Copy link
Contributor Author

I'd vote for AggregateStore not being removed, since one of the nice things about Appdash is that it can be embedded in an application without many dependencies, ie influxdb in this case.

Hi @bg451, thanks for giving ur thoughts about the AggregateStore being removed, would you mind continue the conversation about it in #127, this being is initial comment. This PR is just being build in top of it but it's main purpose is to improve the Recorder.

in order to reduce the number of `InfluxDBStore.Collect()` calls for
performance reasons
@slimsag
Copy link
Member

slimsag commented Apr 16, 2016

LGTM

Let's def. move the AggregateStore conversation elsewhere.

EDIT: see #137

@slimsag slimsag merged commit 374f1c4 into sourcegraph:master Apr 16, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants