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 6 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,75 @@
/**
* 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.HashMap;
import java.util.Map;

public class ExampleStatefulObserver implements TracerObserver {

@Override
public SpanObserver onStart(SpanData spanData) {
return new StatefulSpanObserver(spanData);
}

public static class MyObserverState {
public MyObserverState() {
}
}

public static class StatefulSpanObserver implements SpanObserver {

private Map<Object,MyObserverState> spanState = new HashMap<Object,MyObserverState>();

public StatefulSpanObserver(SpanData spanData) {
MyObserverState state = new MyObserverState();
spanState.put(spanData.getSpanId(), state);
}

@Override
public void onSetOperationName(SpanData spanData, String operationName) {
// TODO Auto-generated method stub

}

@Override
public void onSetTag(SpanData spanData, String name, Object value) {
// TODO Auto-generated method stub

}

@Override
public void onSetBaggageItem(SpanData spanData, String key, String value) {
// TODO Auto-generated method stub

}

@Override
public void onLog(SpanData spanData, long timestampMicroseconds, Map<String, ?> fields) {
// TODO Auto-generated method stub

}

@Override
public void onLog(SpanData spanData, long timestampMicroseconds, String event) {
// TODO Auto-generated method stub

}

@Override
public void onFinish(SpanData spanData, long finishMicros) {
MyObserverState state = spanState.remove(spanData.getSpanId());
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* 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;

public class ExampleStatelessObserver implements TracerObserver, SpanObserver {

@Override
public SpanObserver onStart(SpanData spanData) {
return this;
}

@Override
public void onSetOperationName(SpanData spanData, String operationName) {
// TODO Auto-generated method stub

}

@Override
public void onSetTag(SpanData spanData, String name, Object value) {
// TODO Auto-generated method stub

}

@Override
public void onSetBaggageItem(SpanData spanData, String key, String value) {
// TODO Auto-generated method stub

}

@Override
public void onLog(SpanData spanData, long timestampMicroseconds, Map<String, ?> fields) {
// TODO Auto-generated method stub

}

@Override
public void onLog(SpanData spanData, long timestampMicroseconds, String event) {
// TODO Auto-generated method stub

}

@Override
public void onFinish(SpanData spanData, long finishMicros) {
// TODO Auto-generated method stub

}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/**
* 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;

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

/**
* This method returns an id that can be used for correlation purposes. It should only
* be used within the application to uniquely distinguish one span from another,
* to enable state to be maintained within observer implementation where appropriate.
*
* @return The unique id for the span
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

*/
Object getSpanId();

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

String getOperationName();

/* Spec does not indicate that a tag key could have multiple values - but some tracers support
* this? Would be good to understand the usecase for multivalued keys - and add some text in the
* spec to clarify this?
* Possibly as well as returning the String,Boolean,Number values, it could return a List in those cases?
*
* Q: Do we need a 'getTags' method - in which case would it return Map<String,List<?>> ?
* Other option may be to have a Set<String> getTagKeys() ?
*/
Object getTag(String key);

Object getBaggageItem(String key);

}
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 represents an observer used to receive notifications related to a {@link 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 named tag has been set/changed.
*
* @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 the associated {@link Span} 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,20 @@
*/
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.
*
* @param spanData The data for the span being started
* @return The observer for the {@link Span}
*/
SpanObserver onStart(SpanData spanData);

}