Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial version of Observer APIs #3

Merged
merged 12 commits into from
Jul 10, 2017
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/**
* Copyright 2017 The OpenTracing Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/
package io.opentracing.contrib.observer;

import java.util.Map;

/**
* This interface provides information about the current {@link io.opentracing.Span} to the observer
* methods.
*
*/
public interface SpanData {

/**
* This method returns an id that can be used for correlate actions invoked on a
* stateful observer. It should only be used within the scope of an application,
* to uniquely distinguish one span from another, to enable state to be maintained
* within observer implementation where appropriate.
*
* @return The correlation id for the span, MUST implement equals/hashCode to enable
* it to be used as a map key
*/
Object getCorrelationId();

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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?


/**
* The start time of the {@link io.opentracing.Span}.
*
* @return The start time (in microseconds)
*/
long getStartTime();

/**
* The finish time of the {@link io.opentracing.Span}.
*
* @return The finish time (in microseconds), or 0 if not available
*/
long getFinishTime();

/**
* The duration of the {@link io.opentracing.Span}.
*
* @return The duration (in microseconds), or 0 if the span is not finished
*/
long getDuration();

/**
* The operation name of the {@link io.opentracing.Span}.
*
* @return The operation name
*/
String getOperationName();

/**
* This method provides access to the tags associated with the span.
*
* @return The tags
*/
Map<String,Object> getTags();

/**
* This method retrieves a baggage item associated with the supplied key.
*
* @param key The key
* @return The baggage item, or null if undefined
*/
Object getBaggageItem(String key);

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/**
* Copyright 2017 The OpenTracing Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software distributed under the License
* is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
* or implied. See the License for the specific language governing permissions and limitations under
* the License.
*/
package io.opentracing.contrib.observer;

import java.util.Map;

/**
* This interface represents an observer used to receive notifications related to a {@link io.opentracing.Span}.
*
*/
public interface SpanObserver {

/**
* Notifies the observer that the operation name has been changed.
*
* @param spanData The data for the span
* @param operationName The new operation name
*/
void onSetOperationName(SpanData spanData, String operationName);

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Choose a reason for hiding this comment

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

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


/**
* Notifies the observer that the tag with the supplied key has been set or updated
* on a {@link io.opentracing.Span}.
*
* @param spanData The data for the span
* @param key The tag key
* @param value The tag value
*/
void onSetTag(SpanData spanData, String key, Object value);

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand - do you mean modify the tag values in the observed span?

Choose a reason for hiding this comment

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

I mean modifying or omitting the tag value which is about to be set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is out of scope for an observer which is essentially called after the fact. That sounds more like something a tracer wrapper should do?

Choose a reason for hiding this comment

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

If a wrapper is a superset of an observer lets make a wrapper then ;)

Choose a reason for hiding this comment

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

I think we are best of finding a middle ground - a combination of the wrapper and observer approach. I'm not proposing a full blown wrapper like a servlet filter with a filter chain for each span method. What I'm proposing is a very light-weight approach for modification of tags which does not require a "FilterChain" object:

String onSetTag(String key, String value);

Implementations can now influence the value. They can return it as-is if they don't want to modify it, return a different value or return null to omit adding the tag.

I don't think this has a significantly higher overhead than a "pure" observer and we could handle more use cases with this approach.

One more use case I have is to be able to configure a set of excluded tags.

Copy link
Contributor Author

@objectiser objectiser Jun 29, 2017

Choose a reason for hiding this comment

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

I agree there are usecases, to bridge between framework instrumentations that generate a certain set of tags, and a tracer that is responsible for simply recording those tags. Having a way to plugin a tracer independent mechanism that can pre-process the tags and even logs would be good.

However I still think it is better to have a distinction between Observer and something that could actively take part in controlling what information is being stored.

One possibility is to have both concepts within the same repo - but have distinct APIs - Observer as now, dealing with the span lifecycle and change events, and a PreProcessor (TBD) that only has a few methods to e.g. allow changes to tags and logs?

cc @pavolloffay @jpkrohling @yurishkuro @hkshaw1990

Copy link
Collaborator

Choose a reason for hiding this comment

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

@objectiser will be a new tag present in SpanData when invoking this method?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should clarify this somewhere in the javadoc. It should apply for the rest of the methods too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree this should be defined - my initial thought is that it is best post change, so that these callbacks could be performed asynchronously to not block the app or tracer. If the observer is interested in the previous state then it could cache the previous values itself.


/**
* Notifies the observer that the named baggage item has been set/changed.
*
* @param spanData The data for the span
* @param key The baggage key
* @param value The baggage value
*/
void onSetBaggageItem(SpanData spanData, String key, String value);

/**
* Notifies the observer that a log event has been recorded.
*
* @param spanData The data for the span
* @param timestampMicroseconds The explicit timestamp for the log record. Must be greater than or equal to the
* Span's start timestamp.
* @param fields key:value log fields. Tracer implementations should support String, numeric, and boolean values;
* some may also support arbitrary Objects.
*/
void onLog(SpanData spanData, long timestampMicroseconds, Map<String, ?> fields);

/**
* Notifies the observer that a log event has been recorded.
*
* @param spanData The data for the span
* @param timestampMicroseconds The explicit timestamp for the log record. Must be greater than or equal to the
* Span's start timestamp.
* @param event the event value; often a stable identifier for a moment in the Span lifecycle
*/
void onLog(SpanData spanData, long timestampMicroseconds, String event);

/**
* Notifies the observer that a span associated with the supplied data has finished.
*
* @param spanData The data for the span
* @param finishMicros The finish time in microseconds
*/
void onFinish(SpanData spanData, long finishMicros);

}
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,36 @@
*/
package io.opentracing.contrib.observer;

import io.opentracing.Span;

/**
* This interface represents an observer used to receive notifications related to {@link Span}s.
*
*/
public interface TracerObserver {

/**
* Notifies the observer that a new span has been started with the supplied data.
*
* <p>An observer can either be implemented as stateful or stateless:
*
* <p> * A stateful implementation should return a new instance of a {@link SpanObserver} implementation
* for each span that is started, allowing this implementation to locally maintain
* information about that span as subsequent {@link SpanObserver} methods are called. For example,
* where the observer is interested in a sequence of events associated with the span, such as the
* recording of a new tag initiating a metric which then needs to be completed/recorded when the
* span is finished.
*
* <p> * A stateless implementation should return a singleton instance
* of the {@link SpanObserver}. Each call to the observer will be handled in isolation. For example,
* this approach would be useful when only interested in a specific event - such as
* a {@link SpanObserver#onLog onLog} event which can result in the log details being recorded
* to a logging framework, or {@link SpanObserver#onFinish onFinish}
* being used to record metrics about the duration of the span.
*
* @param spanData The data for the span being started
* @return The observer for the {@link Span}
*/
SpanObserver onStart(SpanData spanData);

}