-
Notifications
You must be signed in to change notification settings - Fork 250
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 TraceFlags class #57
Conversation
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.
We should discuss the best place for the generate_trace_id
and generate_span_id
methods, as they might be needed outside of span creation.
We should also determine what is the best internal format to use for span and trace ids. For context propagation, having the ids in a lowercase hex string format is going to be required, and that may make it a desirable format for export and correlation.
Does it make sense to represent trace and span ids as lowercase hex internally? Are there any situations where we'd want the underlying integer representation instead? If we want the string representation more often than we'd want the integer, we should consider using a string so we do not have to convert the same id to a string in multiple locations.
For reference, OpenTelemetry JS uses the lower hex string representation: https://github.com/open-telemetry/opentelemetry-js/blob/3f90bf97dcf250b8c97e4a2d8db1edbaaf488f68/packages/opentelemetry-core/src/platform/node/id.ts#L21-L35
Python uses integer representations: https://github.com/open-telemetry/opentelemetry-python/blob/4aea780ab04fe93b93b8efca5b956febea050818/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L259-L265
Other projects wrap ids in SpanId and TraceId classes abstracting those details away, and #23 does that if we want to entertain that idea.
It is certainly a reasonable choice. We cheated a little in Shopify's internal tracing project, and used lowercase hex trace IDs, but integers in the range We can use |
I think going with a string for our internal representation is a good approach for now. If it turns we regularly need both string and integer representations, we can consider #23. |
Bumping this comment, in case it was missed.
|
584efe1
to
f35ffbf
Compare
Added tests for |
# @raise [ArgumentError] If flags is not an 8-bit byte | ||
# @return [TraceFlags] | ||
def from_byte(flags) | ||
raise ArgumentError, 'flags must be an 8-bit byte' unless (0..255).include?(flags) |
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 more efficient to unless flags & ~0xff == 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.
The bitwise check is a clear winner:
require 'benchmark/ipsa'
num = 144
Benchmark.ipsa do |x|
x.report 'range check' do
(0..255).include?(num)
end
x.report 'bitwise check' do
num & ~0xFF == 0
end
x.compare!
end
Results:
Allocations -------------------------------------
range check 0/0 alloc/ret 0/0 strings/ret
bitwise check 0/0 alloc/ret 0/0 strings/ret
Warming up --------------------------------------
range check 180.625k i/100ms
bitwise check 205.490k i/100ms
Calculating -------------------------------------
range check 7.869M (± 4.7%) i/s - 39.376M
bitwise check 15.487M (± 2.7%) i/s - 77.470M
Comparison:
bitwise check: 15486958.1 i/s
range check: 7868744.9 i/s - 1.97x slower
This is ready for review. |
api/lib/opentelemetry/trace.rb
Outdated
module Trace | ||
# An invalid trace identifier, a 16-byte array with all zero bytes, encoded | ||
# as a hexadecimal string. | ||
INVALID_TRACE_ID = '0' * 32 |
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.
Should we freeze this string (and INVALID_SPAN_ID below)?
can this be approved now? It looks as though the comments have been addressed? |
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 implement
TraceFlags
. This used to be calledTraceOptions
, but open-telemetry/opentelemetry-specification#234 proposes a rename to match the W3C spec.This also implements
generate_span_id
andgenerate_trace_id
inSpanContext
.