Initial version of Observer APIs

[objectiser]

Initial stab at the observer API. Uses similar approach to https://github.com/opentracing-contrib/go-observer/blob/master/observer.go.

Main differences:

• As Java uses a builder approach, I added an additional SpanBuilderObserver to receive notifications related to the different steps in the builder process. If we want to collapse this into an TracerObserver.onStart method, then we would need to decide how the tags and references would be supplied to the observer impl.
• The SpanObserver also includes methods for receiving notifcations about set/changed baggage and also logs. Baggage would be required by https://github.com/opentracing-contrib/java-metrics, and also potentially information from structured log events.

[objectiser]

cc @hkshaw1990 @bhs @yurishkuro @pavolloffay @jpkrohling

[felixbarny]

Maybe join with SpanObserver? How would implementations know wether a specific tag is set on the SpanBuilder or the Span? If there is only one onSetTag method which is invoked no matter if the tag was actually set on a SpanBuilder or a Span, people don't have to worry about that.

[objectiser]

Do you mean have one interface SpanObserver that is used by the SpanBuilder to notify, so it would potentially call onSetTag and onAddReference before onStart, and then afterwards potentially have onSetTag called other times before finally onFinish?

[felixbarny]

Correct. That's how I implemented this for stagemonitor: https://github.com/stagemonitor/stagemonitor/blob/master/stagemonitor-tracing/src/main/java/org/stagemonitor/tracing/wrapper/SpanEventListener.java

[objectiser]

That would be fine my me - although would like to get other feedback first before applying change.

[felixbarny]

Maybe add Span parameter similar to onStart?

[objectiser]

None of the other methods (apart from onStart) are supplied the Span - what would be the purpose of adding it here?

[felixbarny]

You could use the instance to set additional tags onFinish. Another use case is reporting. See also https://github.com/opentracing-contrib/java-span-reporter/blob/master/span-reporter/src/main/java/io/opentracing/contrib/reporter/Reporter.java#L22

[objectiser]

In both these cases the Observer instance could save the span supplied with the onStart and reuse it when any of the other methods were invoked - although I understand the benefit if an impl was only interested in doing something onFinish it would not require an Observer instance per span.

Currently the go observer only supplies it onStart, so if we wanted to change the approach then would need to address it there aswell.

[felixbarny]

it would not require an Observer instance per span.

That's the key point. In stagemonitor, I have the notion of a StatelessEventListener which is a single instance reused for all spans. This saves a lot of object instantiation and reduces GC overhead imposed by the agent.

Another feature my impl currently supports is altering and omitting of certain tags via the onSetTag method.

[felixbarny]

Note: the operation name can be set multiple times for a span and could also be set via the SpanBuilder. As you only know the operation name for certain when Span#finish is invoked, in my impl onFinish has a operationName parameter.

[objectiser]

@hkshaw1990 @bhs @yurishkuro @pavolloffay @jpkrohling

As highlighted by @felixbarny an important issue to address is whether the SpanObserver should be stateful, stateless or both options supported. Thoughts?

[felixbarny]

My vote would be for both. Stateless observers can easily be implemented via stateful ones. See https://github.com/stagemonitor/stagemonitor/blob/0.80.0.RC1/stagemonitor-tracing/src/main/java/org/stagemonitor/tracing/wrapper/StatelessSpanEventListener.java.

Another key question is wether the WrappedSpan should support readback of tags and other stuff as implemented in java-span-reporter. I think this should not be done in a generic observer/event listener lib, although being able to opt-in readback would be quite handy.

[objectiser]

Agree that stateless can be implemented via stateful - but the issue is the observer API would need to pass the tags and references to some of the callbacks - so would require agreement on how this would be done.

[jpkrohling]

Before giving my vote, would it be possible to add some examples to the code repository, showing the use cases you had in mind?

[yurishkuro]

@tedsuo had a similar comment about stateless observers, with the same caveat - to do stateless we need some sort of read-back mechanism, e.g. for SpanID.

[hkshaw1990]

If I understand this right, we probably would need a stateful span observer (atleast) from the perspective of a perfevents observer. In case of a perfevents observer, some metrics are opened for a span in onStart() and onSetTag(), and those metrics are read and closed in onFinish(). We need to maintain the contexts of those metrics somewhere. Considering that there is no span observer per span (in case of stateless), we would need a unique id for a span, which I am not sure if opentracing or a tracer impl mandates that (correct me if I am wrong). However, since a stateless approach will have smaller overhead, I am open to any suggestions to have a stateless perfevents observer.

[objectiser]

Its true that there is no standard concept of span id in OpenTracing currently, but that wouldn't necessarily prevent the Observer callbacks supplying some form of correlation id that would be provided by the tracer impl, but it would only be for local correlation purposes. You mentioned metrics opened for `onSetTag()` - just curious what metrics would be related to tags?

…

On Tue, Jun 20, 2017 at 3:07 PM, Hemant Kumar ***@***.***> wrote: If I understand this right, we probably would need a stateful span observer (atleast) from the perspective of a perfevents observer. In case of a perfevents observer, some metrics are opened for a span in onStart() and onSetTag(), and those metrics are read and closed in onFinish(). We need to maintain the contexts of those metrics somewhere. Considering that there is no span observer per span (in case of stateless), we would need a unique id for a span, which I am not sure if opentracing or a tracer impl mandates that (correct me if I am wrong). However, since a stateless approach will have smaller overhead, I am open to any suggestions to have a stateless perfevents observer. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKC0kCywTY2eMDevql8gpNOkX5nNE2Eks5sF9IxgaJpZM4N9ebI> .

[hkshaw1990]

@objectiser regarding onSetTag(), I would need to monitor a span for platform (hardware) related metrics, i.e., as soon as a span is started with some tags or if a span is later tagged with something like : sp.SetTag("perfevents", "cpu-cycles,instructions"), in the callback, I would start monitoring these metrics from that point for that span. At onFinish(), these metrics need to be collected and the related file descriptors (event contexts) need to be closed.

[objectiser]

Next iteration - I think it addresses the previous comments - so provides support for stateful/less with a simple example of each.

Each callback has access to the current state of the span via SpanData - so (for example) the finish method could access the latest operation name.

The main issue is how to provide access to tags, when potentially multiple tags could be added with the same key. The spec does not clearly indicate whether this is supported, so wondering what the usecase is here - or whether it is just to support tracers that allow this?
Have added some suggestions in the javadoc for the getTag method - but any suggestions appreciated.

[pavolloffay]

The main issue is how to provide access to tags, when potentially multiple tags could be added with the same key. The spec does not clearly indicate whether this is supported, so wondering what the usecase is here - or whether it is just to support tracers that allow this?

I think there are tracers which support it. SpanData.getTag(String key) could return a collection of values.

Other option may be to have a Set getTagKeys() ?

I think this can be handy. And also something to iterate over all tags?

[jcchavezs]

I am not so sure about the stateful approach for observer since you might keep two states (the span one and the spanObserver one). Are there any ignored tags prefix in the standard? Can the stateful implementation avoid the introduction of the spanId by setting a tag prefixed that is ignored? another option would be that the observer removes that tag in finish but then it would not be just an observer.

[objectiser]

@jcchavezs In general I think the state maintained within a stateful observer will be observer specific - so the example I have added is misleading in that sense - because the stateful observer methods could obtain any span related data from the SpanData.

[yurishkuro]

I would add that the critical requirement for the return value is to implement equals/hashCode to allow it to be used as a map key.

[objectiser]

Have updated the SpanObserver related to tags to just provide Map getTags() - if a tracer does support multiple values per tag then they could just return a List.

[jpkrohling]

LGTM, although I would prefer a different method name for the onSet... methods.

[jpkrohling]

Sorry I missed this on the first review, but wouldn't it be more semantically accurate to have Change instead of Set on the name? onOperationNameChange for instance.

[objectiser]

The naming was supposed to reflect invocation of the method name following the on - so in this case when the setOperationName is called. But I have no problem with using a different convention.

Anyone else have a preference?

[felixbarny]

I actually like onOperationNameChange(d) as it hints that it can be called several times.

[felixbarny]

Have updated the SpanObserver related to tags to just provide Map getTags() - if a tracer does support multiple values per tag then they could just return a List.

How does a implementor of the SpanObserver interface know wether the tracer will return a List or a single value? Also, what if the underlying Tracer implementation is swapped out? That will probably lead to ClassCastExceptions. Maybe the signature should be Map<String, List<?>> and if the tracer supports only one value per tag key, they can return a singleton list.

[pavolloffay]

I have the same concern.

We could return:

• Map<String, List<?>>
• List

I think singletonList would not add too much overhead.

btw. javadoc does not say anything about it)

[felixbarny]

Without opentracing/specification#24, how should this be implemented in a Tracer implementation agnostic way?

[objectiser]

The only purpose of the returned value is to act as a key for looking up information that may have previously be cached for the span. Once the span is finished, it has no other purpose - i.e. it is not a long term reference to gain access to the span.

[felixbarny]

I see. But can't SpanData itself be used as the map key then?

[objectiser]

Its possible, although I would prefer to have it separate. But open to other opinions.

[felixbarny]

Maybe create a separate issue for that? Or should we agree on something before merging? Also, shouldn't the default equals/hashCode implementation (object identity) be good enough?

[pavolloffay]

+1 on previous comments, that observer should only observe things and not change. Changes should be done in filters/interceptors.

Do we want to put ALL these in the same repo?

About the wrapper, certainly it is one possible impl but I thought we want to define an API which could be implemented by the tracer itself.

If ^^ holds I see these two things as extensions to the current API, therefore names like java-api-extensions or java-tracer-extensions make sense to me.

[objectiser]

Do we want to put ALL these in the same repo?

The main reason I was proposing this is because it may be good for both observer and filter to have access to the SpanData.

[felixbarny]

Ok, then we might have a misconception. I thought this repo was about wrapping or decorating (maybe java-tracer-decorator?) a tracer implementation which adds the ability to register observers (and possibly filters/interceptors). I don't think the tracer should implement the interfaces itself but we could rather provide an implementation independent Tracer decorator which also implements the Tracer interface. Then it would make sense to add both observers and interceptors to this repo so that both can use the same decorator foundation.

[objectiser]

@felixbarny I don't think it needs to be one way or the other - a tracer could natively support the observer API but equally we could provide a wrapper as an alternative that takes on the management of observers. It would be good to offer options.

[objectiser]

Have created issues for the areas raised - including renaming the repo, as I think it would be better to get this PR merged first. So essentially there will be two APIs (currently) - Observer and Filter/Interceptor, and a default wrapper impl that provides support for managing these APIs outside the tracer impls.

So to make progress would be good to just focus this PR on completing the observer API. Any comments on that topic welcome.

[pavolloffay]

I think this LGTM, if anyone from OTSC could review it would be 💯

cc @yurishkuro @bhs @felixbarny

[felixbarny]

nit: missing space after comma

[felixbarny]

Maybe create a separate issue for that? Or should we agree on something before merging? Also, shouldn't the default equals/hashCode implementation (object identity) be good enough?

[felixbarny]

LGTM

[bhs]

@pavolloffay I'm happy to take a look (and actually have already taken a look) but felt that the discussion was so in-depth that it might not be helpful to have "another cook in the kitchen", esp since this is OT-contrib.

Are there particular (somewhat-)open questions that would benefit from an additional perspective?

[felixbarny]

@bhs apart from the "spin-off-issues" #7 and #6 I'd like to have another opinion about the modification of tag values via return values from onSetTag and on the Map<String, List<Object>> getTags() method. Is it really intended to be able to store multiple values per tag key? Should we really implement this "one-off" feature which only very few tracers support? Which tracer supports this btw and what is the use case? It makes implementations a bit more difficult, makes it awkward to read a value when expecting only one value per tag key and adds at least another reference (via the singleton list) per tag.

Also, onSetOperationName vs onOperationNameChanged?

[objectiser]

Based on @yurishkuro 's response in opentracing/specification#79 (comment) the Span API implies one tag value per key, even if some tracers may retain previous values - so this is implementation specific.

Therefore it would seem more appropriate that the retrieval of the tags should take the same approach - be optimised for single value retrieval, but cater for multivalue if an implementation supports it and there are actually multiple values for a particular key.

This could be achieved using Map<String,Object> getTags() operation, where the value could be string, bool, number or list - where the list could be of values of the previous 3 types. So a list would only be returned if the tracer impl actually had multiple values for the same tag key.

If the untyped value is not desirable, then Set<Tag> getTags() where the Tag type identifies the key and has methods for returning the typed values. There could also be a Set<Tag> getTag(String key) variant to get values for a particular key. But this still has the drawback of dealing with a set of tags, when most likely only one value will be returned.

Thoughts? Votes?

[pavolloffay]

Ideally, the API for tags should provide:

• iterate over all tags
• ~O(1) access to the given tag

Maybe it's better to go with the simplest approach and just assume there is one tag with the given key.

Multi value tags can be spotted by void onSetTag(SpanData spanData, String key, Object value);, When calling this method SpanData would not contain value. The value will be added after.

[felixbarny]

Map<String,Object> getTags() operation, where the value could be string, bool, number or list - where the list could be of values of the previous 3 types

This is quite problematic as a user of the method you can't be sure wether the tracer will return a List or a String. To write tracer agnostic code they would have to do an instanceof every time.

Iff we want to support multi value tags readback we should probably have two methods - Map<String,Object> getTags() and List<String, List<Object>> getMultiValueTags(). But it would be easier to say that it is not supported in the first version and tracers having support for it have to return the first value.

[objectiser]

Ok sounds like a plan :) - will simplify to Map<String.Object> getTags() - but @felixbarny shouldn't it return the latest value for the tag key, assuming an overwrite approach?

[felixbarny]

but @felixbarny shouldn't it return the latest value for the tag key, assuming an overwrite approach?

you are right!

To start yet another discussion (sorry about that) what about additional typed tag accessor methods?

boolean getBooleanTag();
String getStringTag();
Number getNumberTag();

That would cut down on casts and instanceof checks. Personally, I don't quite like the idea to introduce a Tag type as it would mean another object reference per tag.

[objectiser]

That should be fine - should getStringTag return a string representation of bool or number tags? or only return value if the tag was a string type?

If the requested type is not correct (apart from the above), should the method just return null or an exception?

[felixbarny]

should getStringTag return a string representation of bool or number tags?

No

If the requested type is not correct (apart from the above), should the method just return null or an exception?

Not an easy one... As exceptions in monitoring code can be quite "dangerous" I'd vote for return null.

[objectiser]

@felixbarny @jpkrohling @yurishkuro @hkshaw1990 @pavolloffay Thanks all for the comments.

Further work will be continued in the raised issues - but if there are any further points regarding the current API that need to be discussed, then please raise additional issues. Next step is to rename the repo.