Remove RunningSpanStore and SampledSpanStore from API

[bogdandrutu]

I think this should be moved in the implementation (currently no public users except tests). We should add an "exporter" (haven't decided what is the best name):

public abstract AllSpansExporter {
  public abstract void registerHandler(String name, Handler handler);
  public abstract void unregisterHandler(String name);

  public abstract static class Handler {
    public abstract void onStart(SpanData spanData);
    public abstract void onEnd(SpanData spanData);
  }
}

Then the current logic of RunningSpanStore and SampledSpanStore will be implemented using a ZPagesHandler which extends AllSpansExporter.Handler.

We will move the RunningSpanStore and SampledSpanStore in the zpages

public class ZPagesHandler extends AllSpansExporter.Handler {
  private RunningSpanStore runningSpanStore;
  private SampledSpanStore sampledSpanStore;
  ... 
}

Some other API changes:

• Default value for the recordEvents property will be true if a AllSpansExporter.Handler is registered otherwise false.
• We should think about deprecating/removing the option from EndSpanOptions "sampleToLocalSpanStore". The way I can think of is to implement the SampledSpanStore as LRU and keep the latest 1000 span names.
• For callers of the registerSpanNamesForCollection we should apply the same strategy as "sampleToLocalSpanStore" property.

So the plan will be not directly to remove this classes but deprecate them and start changing callers.

Pros:

• Simpler API that people have to use in their integration: no need to think about "sampleToLocalSpanStore", "registerSpanNamesForCollection" or "recordEvents".
• Allows us (and others) to implement other functionalities (e.g. keep all spans in a buffer locally for later query support).

Cons:

• API changes and deprecation process. The good news is that the APIs are not used to much and the proposed change for "sampleToLocalSpanStore" and "registerSpanNamesForCollection" is backwards compatible in a way that we can just make the old APIs be no-op.

[bogdandrutu]

/cc @SergeyKanzhelev

[codefromthecrypt]

I think this is very like "firehose mode" handler discussed last meeting as all data goes here. In brave (and py zipkin) we are thinking about literally calling this firehose though all spans also conveys the same.

I really prefer this as i didnt really like the hooks in end span options anyway. We have a lot more flexibility in what we can create with allspansexporter

On 8 Feb 2018 12:49 am, "Bogdan Drutu" [email protected] wrote:

I think this should be moved in the implementation (currently no public users except tests). We should add an "exporter" (haven't decided what is the best name):

public abstract AllSpansExporter { public abstract void registerHandler(String name, Handler handler); public abstract void unregisterHandler(String name);

public abstract static class Handler { public abstract void onStart(SpanData spanData); public abstract void onEnd(SpanData spanData); } }

Then the current logic of RunningSpanStore and SampledSpanStore will be implemented using a ZPagesHandler which extends AllSpansExporter.Handler.

We will move the RunningSpanStore and SampledSpanStore in the zpages

public class ZPagesHandler extends AllSpansExporter.Handler { private RunningSpanStore runningSpanStore; private SampledSpanStore sampledSpanStore; ... }

Some other API changes:

• Default value for the recordEvents property will be true if a AllSpansExporter.Handler is registered otherwise false.
• We should think about deprecating/removing the option from EndSpanOptions "sampleToLocalSpanStore". The way I can think of is to implement the SampledSpanStore as LRU and keep the latest 1000 span names.
• For callers of the registerSpanNamesForCollection we should apply the same strategy as "sampleToLocalSpanStore" property.

So the plan will be not directly to remove this classes but deprecate them and start changing callers.

Pros:

• Simpler API that people have to use in their integration: no need to think about "sampleToLocalSpanStore", "registerSpanNamesForCollection" or "recordEvents".
• Allows us (and others) to implement other functionalities (e.g. keep all spans in a buffer locally for later query support).

Cons:

• API changes and deprecation process. The good news is that the APIs are not used to much and the proposed change for "sampleToLocalSpanStore" and "registerSpanNamesForCollection" is backwards compatible in a way that we can just make the old APIs be no-op.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/census-instrumentation/opencensus-java/issues/981, or mute the thread https://github.com/notifications/unsubscribe-auth/AAD618sQ3hJ6ger69DFkaBjEzXPKb1DQks5tSjZygaJpZM4R9lLD .

[SergeyKanzhelev]

What's the difference between AllSpanExporter and Exporter? Why Exporter cannot be just an AllSpanExporter with the "sampling" filter implemented as part of it?

[HailongWen]

IMO AllSpanExporter and existing Exporter do share the same functionality of exposing/exporting the local span data for others to use. There do exist some minor differences. An exporter is often considered to actively push data to other sources in a fixed interval, while the proposed one just stores data locally and waits to be pulled. An exporter should try to ensure data integrity (e.g. flush on crash) while the latter one can be fragile, not so accurate.

So yes I think we can implement one using another if we really want to. For example, implement existing SpanExporter as a AllSpanExporter that has no runningSpanStore and does nothing when onStart is called. But if we really want to separate them, we'd better pick another name to avoid confusion. I am thinking about something like Collector to emphasize on the collecting behavior rather than exportation.

I think what bogdan is trying to do is to decouple/defer the option of recordEvent/sampleToLocal in tracing API and move these logic to other exportation related components. It is really hard for user to tell whether a span should be recorded during compilation. The added API might provide a way for user to configure it before run or even adjust in runtime.

[SergeyKanzhelev]

@HailongWen this is exactly the question. Are these two separate concepts necessary? I'm trying to understand the benefit of exporter comparing to allspanexporter.

[codefromthecrypt]

From a config pov it is easier to use presence of an 'all handler' to know if you should add overhead to all requests vs those just sampled for export or local storage. The type could be the same but there is value knowing the difference between the firehose and a sampled stream of it.

[bogdandrutu]

There are 2 use-cases that I have in my mind (very similar to what @adriancole said):

1. User wants to export traces that are sampled in advance (classic sampling at the beginning of the trace/span). In this case we can optimize and drop all events for not sampled spans immediately.
2. User wants to export all spans to the z-pages or other handlers. In this case we need to collect all the events for all the spans.

I think that a lot of our users will just want the first option and supporting that optimal is probably a good thing.

We can have one exporter interface but we can have a method on the Handler which says "need access to all spans or just sampled".

@SergeyKanzhelev does this make sense to you?