-
Notifications
You must be signed in to change notification settings - Fork 825
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
Add BinaryFormat interface and implementation #74
Conversation
qq, where does the binary format for w3c trace context come from? is it compatible with, say, the java implementation? |
* | `---------------------------------- traceID field ID (0) | ||
* `------------------------------------ version (0) | ||
*/ | ||
const result = Buffer.alloc(FORMAT_LENGTH, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
allocUnsafe
is a lot faster when the buffer is written to right away.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While there are performance advantages to using Buffer.allocUnsafe()
, the new Buffer is uninitialized (the allocated segment of memory might contain old data - tests are failing because of that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. Since data is written to the buffer right away on the next few lines, the original data of the buffer should be irrelevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use only ArrayBuffer
in this code?
4 * ID_SIZE + TRACE_ID_SIZE + SPAN_ID_SIZE + TRACE_OPTION_SIZE; | ||
|
||
export class BinaryTraceContext implements BinaryFormat { | ||
toBytes(spanContext: SpanContext): Buffer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this class. Isn't it a propagator that should have an inject
and extract
method? If not, why the difference and how is this supposed to be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, there could have an interface that is compatible with other propagators with something like this:
inject(spanContext: SpanContext, format: string, carrier: Injector) {
And then the injector would be a function/class passed by the user that would take the buffer as input. Then they can control how to add the buffer created by OpenTelemetry in their own existing buffer, while keeping the API compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is based on specs here https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/propagators-api.md#propagators-api. Even I was confused about these two interfaces (BinaryFormat
and HTTPTextFormat
), but decided to have something ready for discussion.
@bogdandrutu Could you please address these questions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be done similar to OpenTracing, but with an ArrayBuffer
to support the browser too:
interface BinaryCarrier {
buffer?: ArrayBuffer
}
class BinaryPropagator implements Propagator {
inject(spanContext: SpanContext, format: string, carrier: BinaryCarrier): this { ... }
extract(format: string, carrier: BinaryCarrier): SpanContext | null { ... }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure makes sense to me, will wait for open-telemetry/opentelemetry-specification#174 to get accepted. Mean time, Could you please review again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall correctly, the reason in OpenCensus that this was a Buffer
was to support gRPC, which isn't relevant in the browser case (there is gRPC web but its transport is actually different from native gRPC and it uses a proxy that speaks HTTP actually)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Buffer
can be initialized from an ArrayBuffer
in Node and same for a TypedArray
in the browser. The idea then would be to use an ArrayBuffer
which is compatible with both platforms, and assign it to an object instead of returning it to make it compatible with inject
and extract
.
I think we can move forward with these APIs in the meantime while open-telemetry/opentelemetry-specification#174 and open-telemetry/opentelemetry-specification#165 are being discussed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mayurkale22 could you make this an ArrayBuffer
per our conversation in the SIG today?
It is browser compatible and is efficiently convertable to a Node Buffer
via Buffer.from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, PTAL
This is copied from OpenCensus specs and it is compatible with java's implementation. Having said that, users/vendor can write their own implementation (sdk-level) based on |
These wouldn't be available to users without vendor lock in. The declarative approach of |
* | `---------------------------------- traceID field ID (0) | ||
* `------------------------------------ version (0) | ||
*/ | ||
const result = Buffer.alloc(FORMAT_LENGTH, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean. Since data is written to the buffer right away on the next few lines, the original data of the buffer should be irrelevant.
4 * ID_SIZE + TRACE_ID_SIZE + SPAN_ID_SIZE + TRACE_OPTION_SIZE; | ||
|
||
export class BinaryTraceContext implements BinaryFormat { | ||
toBytes(spanContext: SpanContext): Buffer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Buffer
can be initialized from an ArrayBuffer
in Node and same for a TypedArray
in the browser. The idea then would be to use an ArrayBuffer
which is compatible with both platforms, and assign it to an object instead of returning it to make it compatible with inject
and extract
.
I think we can move forward with these APIs in the meantime while open-telemetry/opentelemetry-specification#174 and open-telemetry/opentelemetry-specification#165 are being discussed
c60ffae
to
a45d6b0
Compare
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
==========================================
- Coverage 100% 99.26% -0.74%
==========================================
Files 12 14 +2
Lines 200 271 +71
Branches 8 20 +12
==========================================
+ Hits 200 269 +69
- Misses 0 2 +2
|
a45d6b0
to
531c497
Compare
@rochdev charges have been made as per discussion in the SIG meeting (Use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The API is problematic for vendors but this should be addressed in the spec.
531c497
to
4de8849
Compare
SGTM, will update the interface once we have agreed on the specs. |
Specs: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/propagators-api.md#binary-format