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

Add TraceFlags class #57

Merged
merged 10 commits into from
Sep 5, 2019
Merged

Add TraceFlags class #57

merged 10 commits into from
Sep 5, 2019

Conversation

fbogsany
Copy link
Contributor

@fbogsany fbogsany commented Sep 3, 2019

This implement TraceFlags. This used to be called TraceOptions, but open-telemetry/opentelemetry-specification#234 proposes a rename to match the W3C spec.

This also implements generate_span_id and generate_trace_id in SpanContext.

Copy link
Member

@mwear mwear left a 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.

api/lib/opentelemetry/trace/span_context.rb Outdated Show resolved Hide resolved
api/lib/opentelemetry/trace/trace_flags.rb Show resolved Hide resolved
@fbogsany
Copy link
Contributor Author

fbogsany commented Sep 3, 2019

Does it make sense to represent trace and span ids as lowercase hex internally?

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 1..(2**62 - 1) for span IDs. The latter are generated more often and can be stored more efficiently (small integer).

We can use Random::DEFAULT.bytes(16).unpack1('H*') to generate trace IDs, however we have to wrap this in a loop, checking for all 0.

@mwear
Copy link
Member

mwear commented Sep 3, 2019

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.

@fbogsany
Copy link
Contributor Author

fbogsany commented Sep 3, 2019

Bumping this comment, in case it was missed.

I considered this, but couldn't find a convenient and obvious place for them. We don't currently have any methods on the OpenTelemetry::Trace module, for example, and the OpenTelemetry::Trace::Tracer class is meant to be a no-op implementation. Should we make it part of the Tracer API, and allow alternative implementations to provide their own?

@fbogsany
Copy link
Contributor Author

fbogsany commented Sep 4, 2019

Added tests for SpanContext to exercise these codepaths, and fixed the bugs they uncovered.

# @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)
Copy link
Contributor Author

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?

Copy link
Member

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

@fbogsany
Copy link
Contributor Author

fbogsany commented Sep 4, 2019

This is ready for review.

module Trace
# An invalid trace identifier, a 16-byte array with all zero bytes, encoded
# as a hexadecimal string.
INVALID_TRACE_ID = '0' * 32
Copy link
Member

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)?

@ibawt
Copy link
Contributor

ibawt commented Sep 5, 2019

can this be approved now? It looks as though the comments have been addressed?

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

Totally @ibawt. Thanks for the work @fbogsany!

@fbogsany fbogsany merged commit c1a609e into open-telemetry:master Sep 5, 2019
@fbogsany fbogsany deleted the trace-flags branch September 5, 2019 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants