Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

Simple layer on top of ByteBuffer for BINARY format. #276

Merged
Merged
Show file tree
Hide file tree
Changes from 2 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,48 @@
/*
* Copyright 2016-2018 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.propagation;

import java.nio.ByteBuffer;

public final class Adapters {
Copy link
Member

Choose a reason for hiding this comment

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

what is the benefit of putting helpers here instead of BinaryAdapters.injectCarrier()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I remember we had this idea of putting all the adapters in a single place (including the TextMap ones) - else, I can definitely make BinaryAdapters public, and remove the Adapters class.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind a shared class, but I'm bothered by the name "injectBinary", it reads like a command. We should come up with a more intuitive name, eg injectionCarrier, which works better with BinaryAdapters class.

private Adapters() {}

/**
* Creates an outbound Binary instance used for injection,
* allocating a new ByteBuffer instance when
* setInjectBufferLength() is called. The ByteBuffer can
* be later retrieved using injectBuffer().
*
* @return The new Binary carrier used for injection.
*/
public static Binary injectBinary() {
return new BinaryAdapters.BinaryInjectAdapter();
}

/**
* Creates an inbound Binary instance used for extraction with the
* specified ByteBuffer as input.
*
* @param buffer The ByteBuffer used as input.
*
* @return The new Binary carrier used for extraction.
*/
public static Binary extractBinary(ByteBuffer buffer) {
if (buffer == null) {
throw new NullPointerException();
}

return new BinaryAdapters.BinaryExtractAdapter(buffer);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Copyright 2016-2018 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.propagation;

import java.nio.ByteBuffer;

/**
* Binary is an interface defining the required operations for a binary carrier for
* Tracer.inject() and Tracer.extract(). Binary can be defined either as inbound (extraction)
* or outbound (injection).
*
* When Binary is defined as inbound, extractBuffer() will be called to retrieve the ByteBuffer
* containing the data used for SpanContext extraction.
*
* When Binary is defined as outbound, setInjectBufferLength() will be called in order to hint

Choose a reason for hiding this comment

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

There is no setInjectBufferLenght() method defined in the interface, so it seems to me the documentation does not match the implementation here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, definitely. Will update, thanks for the observation.

* the required buffer length to inject the SpanContext, and injectBuffer() will be called
* afterwards to retrieve the actual ByteBuffer used for the SpanContext injection.
*
* @see Format.Builtin#BINARY
* @see io.opentracing.Tracer#inject(SpanContext, Format, Object)
* @see io.opentracing.Tracer#extract(Format, Object)
*/
public interface Binary {
/**
* Hints the buffer length required for SpanContext injection.
* The user may use this to allocate the ByteBuffer
* or resize an existing one. This method is always called
* before injectBuffer().
*
* It is an error to call this method when Binary is used
* for SpanContext extraction. It is also an error to call
* this method more than one time.
*
* @param length The buffer length required for SpanContext injection.
* It needs to be larger than zero.
*/
void setInjectBufferLength(int length);

/**
* Gets the buffer containing the data used for SpanContext injection.
*
* It is an error to call this method when Binary is used
* for SpanContext extraction.
*
* @return The buffer used for SpanContext injection.
*/
ByteBuffer injectBuffer();
Copy link
Member

Choose a reason for hiding this comment

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

maybe injectionBuffer to mirror injectionCarrier in the adapter?


/**
* Gets the buffer containing the data used for SpanContext extraction.
*
* It is an error to call this method when Binary is used
* for SpanContext injection.
*
* @return The buffer used for SpanContext extraction.
*/
ByteBuffer extractBuffer();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/*
* Copyright 2016-2018 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.propagation;

import java.nio.ByteBuffer;

final class BinaryAdapters {

private BinaryAdapters() {}

public static class BinaryExtractAdapter implements Binary {
Copy link
Member

Choose a reason for hiding this comment

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

does it have to be public?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the current way, yes. But we can hide them through a single method (as we do now for Adapters ;) )

ByteBuffer buffer;

public BinaryExtractAdapter(ByteBuffer buffer) {
this.buffer = buffer;
}

@Override
public void setInjectBufferLength(int length) {
throw new UnsupportedOperationException();
}

@Override
public ByteBuffer injectBuffer() {
throw new UnsupportedOperationException();
}

@Override
public ByteBuffer extractBuffer() {
return buffer;
}
}

public static class BinaryInjectAdapter implements Binary {
ByteBuffer buffer;

@Override
public void setInjectBufferLength(int length) {
if (length < 1) {
throw new IllegalArgumentException("length needs to be larger than 0");
}
if (buffer != null) {
throw new IllegalStateException("injectBuffer() length has already been set.");
}

buffer = ByteBuffer.allocate(length);
}

@Override
public ByteBuffer injectBuffer() {
if (buffer == null) {
throw new IllegalStateException("setInjectBufferLength() needs to be called before injectBuffer()");
}

return buffer;
}

@Override
public ByteBuffer extractBuffer() {
throw new UnsupportedOperationException();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
import io.opentracing.SpanContext;
import io.opentracing.Tracer;

import java.nio.ByteBuffer;

/**
* Format instances control the behavior of Tracer.inject and Tracer.extract (and also constrain the type of the
* carrier parameter to same).
Expand Down Expand Up @@ -76,7 +74,7 @@ private Builtin(String name) {
* @see io.opentracing.Tracer#extract(Format, Object)
* @see Format
*/
public final static Format<ByteBuffer> BINARY = new Builtin<ByteBuffer>("BINARY");
public final static Format<Binary> BINARY = new Builtin<Binary>("BINARY");

/**
* @return Short name for built-in formats as they tend to show up in exception messages.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Copyright 2016-2018 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.propagation;

import java.nio.ByteBuffer;
import org.junit.Test;

import static org.junit.Assert.assertArrayEquals;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;

public class AdaptersTest {

@Test
public void testExtractBinary() {
ByteBuffer buff = ByteBuffer.wrap(new byte[0]);
Binary binary = Adapters.extractBinary(buff);
assertEquals(buff, binary.extractBuffer());
}

@Test(expected = NullPointerException.class)
public void testExtractBinaryNull() {
Adapters.extractBinary(null);
}

@Test(expected = UnsupportedOperationException.class)
public void testExtractBinaryInjectBuffer() {
Binary binary = Adapters.extractBinary(ByteBuffer.allocate(1));
binary.injectBuffer();
}

@Test
public void testInjectBinary() {
Binary binary = Adapters.injectBinary();
binary.setInjectBufferLength(1);
assertNotNull(binary.injectBuffer());
assertEquals(0, binary.injectBuffer().position());
assertEquals(1, binary.injectBuffer().capacity());
}

@Test(expected = IllegalArgumentException.class)
public void testInjectBinaryInvalidLength() {
Binary binary = Adapters.injectBinary();
binary.setInjectBufferLength(0);
}

@Test(expected = IllegalStateException.class)
public void testInjectBinaryNoLength() {
Binary binary = Adapters.injectBinary();
binary.injectBuffer();
}

@Test(expected = UnsupportedOperationException.class)
public void testInjectBinaryExtractBuffer() {
Binary binary = Adapters.injectBinary();
binary.extractBuffer();
}
}
83 changes: 83 additions & 0 deletions opentracing-mock/src/main/java/io/opentracing/mock/MockTracer.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@
*/
package io.opentracing.mock;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
Expand All @@ -25,6 +31,7 @@
import io.opentracing.SpanContext;
import io.opentracing.Tracer;
import io.opentracing.noop.NoopScopeManager;
import io.opentracing.propagation.Binary;
import io.opentracing.propagation.Format;
import io.opentracing.propagation.TextMap;
import io.opentracing.util.ThreadLocalScopeManager;
Expand Down Expand Up @@ -112,6 +119,82 @@ public <C> MockSpan.MockContext extract(Format<C> format, C carrier) {
}
};

Propagator BINARY = new Propagator() {
static final int BUFFER_SIZE = 128;

@Override
public <C> void inject(MockSpan.MockContext ctx, Format<C> format, C carrier) {
if (carrier instanceof Binary) {
Binary binary = (Binary) carrier;
ByteArrayOutputStream stream = new ByteArrayOutputStream();
ObjectOutputStream objStream = null;
try {
objStream = new ObjectOutputStream(stream);
objStream.writeLong(ctx.spanId());
objStream.writeLong(ctx.traceId());

for (Map.Entry<String, String> entry : ctx.baggageItems()) {
objStream.writeUTF(entry.getKey());
objStream.writeUTF(entry.getValue());
}
objStream.flush(); // *need* to flush ObjectOutputStream.

byte[] buff = stream.toByteArray();
binary.setInjectBufferLength(buff.length);
binary.injectBuffer().put(buff);

} catch (IOException e) {
throw new RuntimeException("Corrupted state");
Copy link
Member

Choose a reason for hiding this comment

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

should wrap the cause

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. Will do.

} finally {
if (objStream != null) {
try { objStream.close(); } catch (Exception e2) {}
}
}
} else {
throw new IllegalArgumentException("Unknown carrier");
Copy link
Member

Choose a reason for hiding this comment

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

I would move this to the top to reduce nesting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do as well.

}
}

@Override
public <C> MockSpan.MockContext extract(Format<C> format, C carrier) {
Long traceId = null;
Long spanId = null;
Map<String, String> baggage = new HashMap<>();

if (carrier instanceof Binary) {
Binary binary = (Binary) carrier;
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
ObjectInputStream objStream = null;
try {
ByteBuffer extractBuff = binary.extractBuffer();
byte[] buff = new byte[extractBuff.remaining()];
extractBuff.get(buff);

objStream = new ObjectInputStream(new ByteArrayInputStream(buff));
spanId = objStream.readLong();
traceId = objStream.readLong();

while (objStream.available() > 0) {
baggage.put(objStream.readUTF(), objStream.readUTF());
}
} catch (IOException e) {
Copy link
Member

Choose a reason for hiding this comment

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

rethrow as wrapped?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I'm wondering about this one - the TEXT_MAP format returns nothing (null) if the context is malformed (i.e. trace_id but no span_id, or nothing at all, etc). Should we re-throw the Exception anyway here, though?

Copy link
Member

Choose a reason for hiding this comment

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

I think for mock tracer it's better to throw on malformed carrier. Null is only when http header is missing (which doesn't apply to binary codecs).

} finally {
if (objStream != null) {
try { objStream.close(); } catch (Exception e2) {}
}
}
} else {
throw new IllegalArgumentException("Unknown carrier");
Copy link
Member

Choose a reason for hiding this comment

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

I would move this to the top to reduce nesting. Also clarify "expecting Binary, received " + carrier.getClass()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed ;)

}

if (traceId != null && spanId != null) {
return new MockSpan.MockContext(traceId, spanId, baggage);
}

return null;
}
};

Propagator TEXT_MAP = new Propagator() {
public static final String SPAN_ID_KEY = "spanid";
public static final String TRACE_ID_KEY = "traceid";
Expand Down
Loading