http_text_format + w3c trace parent #55

ibawt · 2019-08-29T14:03:24Z

w3c Trace Parent support

omitting trace state as it's being removed ( at least as of now )
the actual integration points aren't well defined right now, but the TraceParent thing should be alright
I went through the w3c SHOULD's and MUST's and did most of them, at least in terms for the actual parsing

questions

pass through services? apparently we shouldn't do anything with it, not sure how to represent in the current api ( what defines a pass through service? why would anyone write a proxy in ruby anyhoo )
are trace-ids every not 32 bytes? there's language in the w3c standard indicating we should fill them with random values and not zeros. However it also says the trace-id is exactly 32 bytes long.
is this too many objects etc?

https://www.w3.org/TR/trace-context/

ibawt · 2019-09-03T16:37:48Z

@fbogsany 👀 lets start the bike shed of the actual http_text_format api.

api/test/opentelemetry/distributed_context/propagation/trace_parent_test.rb

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb

ibawt · 2019-09-03T17:09:37Z

@fbogsany we could also just not parse the headers at and leave teh trace-ids as hex encoded strings? it would require 2x the memory, but would omit serialization and deserializations. Kind of feel like it should be a Number. maybe we should profile first.

fbogsany · 2019-09-03T17:20:49Z

we could also just not parse the headers at and leave teh trace-ids as hex encoded strings?

See discussion in #57. A similar point was raised there. I don't mind leaving them as hex strings, but we should still validate them.

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb

api/lib/opentelemetry/distributed_context/propagation/http_text_format.rb

ibawt · 2019-09-05T13:57:43Z

@fbogsany comments have been addressed

api/lib/opentelemetry/distributed_context/propagation/binary_format.rb

api/lib/opentelemetry/distributed_context/propagation/http_text_format.rb

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb

fbogsany · 2019-09-05T17:00:51Z

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb

+        end
+
+        def parse_flags(string)
+          OpenTelemetry::Trace::TraceFlags.from_byte(string.to_i(16))


D'you think we should add a from_string to TraceFlags and push the responsibility there?

does the opentelemetry spec list the number of bytes and the hex format? ie 2 digit hex.
if it lives in the w3c but not in the OT spec I would say that would govern where it should live.

The OT spec says TraceFlags is a single byte of boolean flags. Otherwise it defers to the W3C spec.

then it should live here, as this is using a hex encoding of a byte. Therefore the at the boundary it should be a native byte.

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb

api/test/opentelemetry/distributed_context/propagation/trace_parent_test.rb

mwear

Thanks for taking this work on @ibawt. I left a couple comments. I think the diff would be cleaned up if you could rebase this off of master.

mwear · 2019-09-06T18:48:17Z

api/lib/opentelemetry/distributed_context/propagation/http_text_format.rb

@@ -17,7 +16,22 @@ module Propagation
      # Propagation is usually implemented via library-specific request interceptors, where the client-side injects values
      # and the server-side extracts them.
      class HTTPTextFormat
-        # TODO
+        def extract(carrier, &block)


Would an implicit block be better since we opt for yielding and not using the explicit &block param.

In either case can we document the parameters to this method as well?

imo with an implicit block you have to read the documentation/implementation rather than the function signature. If you feel strongly I don't mind.

documentation for sure.

I did the changes, have a look. Not so sure about my documentation though.

Looks good. The alternative would have been to use the block param (block.call) instead of using yield, but its slightly faster to use yield with an implicit block.

So I looked through the rails source, and I think a good rule is, only name the block if we pass it down to another method. Then it's clear the yield is getting "passed" as it were.

api/lib/opentelemetry/distributed_context/propagation/http_text_format.rb

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb

ibawt · 2019-09-09T12:54:36Z

re: rebase, now you know why I was poking at getting the other merged :D

fbogsany · 2019-09-09T18:13:42Z

api/lib/opentelemetry/trace/trace_flags.rb

@@ -25,6 +25,10 @@ def from_byte(flags)
        end
      end

+      def to_s


Add a test?

so I moved this into the trace parent, as the trace parent defines the flags it can support due to it's version. It also says that we can only send flags we know about re: version.

Also this pushes the "encoding" choice to the parent, which I think is the correct place for it.

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb

ibawt · 2019-09-10T13:26:50Z

RE: http_text_format class, I'm not too sold on the "getter" and "setter" as blocks. But I'd like to see how this will integrate with the rest of the code before bike shedding on it too much. Hence most of the meat is in the trace_parent class.

api/lib/opentelemetry/distributed_context/propagation/http_text_format.rb

fbogsany · 2019-09-10T16:26:25Z

api/lib/opentelemetry/distributed_context/propagation/http_text_format.rb

+        # @yield [Carrier, String] the header key
+        # {SpanContext}
+        def extract(carrier)
+          raise Error, 'block must be supplied' unless block_given?


I think this should be an ordinary ArgumentError rather than the OT Error. Same thing below in inject.

fbogsany

LGTM - thanks @ibawt!

mwear · 2019-09-10T18:45:41Z

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb

+      # A TraceParent is an implementation of the W3C trace context specification
+      # https://www.w3.org/TR/trace-context/
+      # {SpanContext}
+      class TraceParent


The layout of this class is not what I am used to seeing. I'm not sure if we have a convention, or if we should have one, but typically I'm used to seeing something like:

class MyClass #constants MY_CONST = 1 class << self #public class methods def public_class_method; end private #private class methods def private_class_method; end end #public instance methods def public_instance_method; end private #private instance methods def private_instance_method; end end

Would there be any objections to adopting a similar format for OTel Ruby (and this file)?

so you want the "static" methods up top? That doesn't bother me at all, I'll move it up.

Yeah, there are some that are in the class << self block and a few that are self. prefixed. If you wouldn't mind moving them into one class << self block at the top that'd be awesome!

I updated it, have a look.

Looks great!

mwear

Just the question about formatting. I also wanted to mention: #73 as it's related (but can and should be a seperate PR).

…emetry-ruby into http-text-format

mwear

Nice work!

mwear · 2019-09-11T18:15:46Z

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb

+      # A TraceParent is an implementation of the W3C trace context specification
+      # https://www.w3.org/TR/trace-context/
+      # {SpanContext}
+      class TraceParent


Looks great!

ibawt commented Sep 3, 2019

View reviewed changes

api/test/opentelemetry/distributed_context/propagation/trace_parent_test.rb Outdated Show resolved Hide resolved

ibawt commented Sep 3, 2019

View reviewed changes

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb Outdated Show resolved Hide resolved

fbogsany reviewed Sep 3, 2019

View reviewed changes

ibawt force-pushed the http-text-format branch from e7046b5 to 927aa38 Compare September 5, 2019 13:40

ibawt force-pushed the http-text-format branch 4 times, most recently from 7d9ff6f to ff8440d Compare September 5, 2019 16:22

fbogsany requested changes Sep 5, 2019

View reviewed changes

ibawt changed the title ~~rough cut~~ http_text_format + w3c trace parent Sep 6, 2019

ibawt marked this pull request as ready for review September 6, 2019 17:55

ibawt requested review from bai, luvtechno, mwear and SergeyKanzhelev as code owners September 6, 2019 17:55

mwear requested changes Sep 6, 2019

View reviewed changes

ibawt force-pushed the http-text-format branch from 71dd7da to fabeaee Compare September 9, 2019 13:23

http_text_format

e0ced19

ibawt force-pushed the http-text-format branch from fabeaee to e0ced19 Compare September 9, 2019 13:33

docs + implicit block

94dbd6d

fbogsany reviewed Sep 9, 2019

View reviewed changes

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb Show resolved Hide resolved

fbogsany reviewed Sep 9, 2019

View reviewed changes

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb Outdated Show resolved Hide resolved

fbogsany reviewed Sep 9, 2019

View reviewed changes

api/lib/opentelemetry/distributed_context/propagation/trace_parent.rb Outdated Show resolved Hide resolved

this should be a .to_s and I should have a test

030cade

ibawt added 6 commits September 10, 2019 09:12

move the encoding choice of the flag into the traceparent

2c8db28

rubocopped

b1fbec8

add url

72bf7d7

spelling

c9beea0

spacing

5c0fbf6

add a bunch of documentation

2002f30

fbogsany reviewed Sep 10, 2019

View reviewed changes

api/lib/opentelemetry/distributed_context/propagation/http_text_format.rb Outdated Show resolved Hide resolved

fbogsany reviewed Sep 10, 2019

View reviewed changes

api/lib/opentelemetry/distributed_context/propagation/http_text_format.rb Outdated Show resolved Hide resolved

fbogsany reviewed Sep 10, 2019

View reviewed changes

ibawt added 2 commits September 10, 2019 12:28

use argument error

5ae9cc2

use the @return

811480e

fbogsany approved these changes Sep 10, 2019

View reviewed changes

Merge branch 'master' into http-text-format

f76e980

mwear reviewed Sep 10, 2019

View reviewed changes

ibawt added 2 commits September 11, 2019 13:50

move up the self methods into one block

859d38a

Merge branch 'http-text-format' of https://github.com/Shopify/opentel…

c7fc1c4

…emetry-ruby into http-text-format

mwear approved these changes Sep 11, 2019

View reviewed changes

fbogsany approved these changes Sep 12, 2019

View reviewed changes

Merge branch 'master' into http-text-format

a68c346

fbogsany merged commit 02f3138 into open-telemetry:master Sep 12, 2019

fbogsany deleted the http-text-format branch September 12, 2019 01:28

fbogsany mentioned this pull request Sep 16, 2019

API: implement HTTPTextFormat #30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http_text_format + w3c trace parent #55

http_text_format + w3c trace parent #55

ibawt commented Aug 29, 2019 •

edited

Loading

ibawt commented Sep 3, 2019

ibawt commented Sep 3, 2019

fbogsany commented Sep 3, 2019

ibawt commented Sep 5, 2019

fbogsany Sep 5, 2019

ibawt Sep 6, 2019

fbogsany Sep 10, 2019

ibawt Sep 10, 2019

mwear left a comment

mwear Sep 6, 2019

ibawt Sep 9, 2019

ibawt Sep 9, 2019

mwear Sep 9, 2019

ibawt Sep 10, 2019

ibawt commented Sep 9, 2019

fbogsany Sep 9, 2019

ibawt Sep 10, 2019 •

edited

Loading

ibawt commented Sep 10, 2019

fbogsany Sep 10, 2019

fbogsany left a comment

mwear Sep 10, 2019

ibawt Sep 11, 2019

mwear Sep 11, 2019

ibawt Sep 11, 2019

mwear Sep 11, 2019

mwear left a comment

mwear left a comment

mwear Sep 11, 2019

http_text_format + w3c trace parent #55

http_text_format + w3c trace parent #55

Conversation

ibawt commented Aug 29, 2019 • edited Loading

w3c Trace Parent support

questions

ibawt commented Sep 3, 2019

ibawt commented Sep 3, 2019

fbogsany commented Sep 3, 2019

ibawt commented Sep 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibawt commented Sep 9, 2019

Choose a reason for hiding this comment

ibawt Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

ibawt commented Sep 10, 2019

Choose a reason for hiding this comment

fbogsany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwear left a comment

Choose a reason for hiding this comment

mwear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibawt commented Aug 29, 2019 •

edited

Loading

ibawt Sep 10, 2019 •

edited

Loading