Fix 'sa' annotation encoding and remove the ':service_port' configuration #139
Conversation
|
thanks, we still need to check to make sure thrift encodes that boolean properly. because I am not that familiar with infrastructure, it took me a lot of manual work to verify the other pull request. Maybe you know a quick way to test this to ensure the remote endpoint ends up ok (ex by looking at result of https://github.com/openzipkin/zipkin-ruby-example with this patched version, but using thrift to send not json) |
|
one way to test is to make a test before this one that compares the result of serializing a span with "sa" to thrift vs a hex encoded value. Then, just make sure that still passes after this one. |
|
We deleted the thrift dependency some versions ago and noone has complained so I do not thing anyone was using it. |
|
We deleted the thrift dependency some versions ago and noone has
complained so I do not thing anyone was using it.
I thought kafka still uses thrift encoding.. is that not the case? If it is
by accident, we should switch it to json..
…On Wed, Feb 27, 2019 at 2:24 PM Jordi Polo Carres ***@***.***> wrote:
We deleted the thrift dependency some versions ago and noone has
complained so I do not thing anyone was using it.
We are talking about Ruby here, performance is never the first
consideration, I'd be shocked when someone opens an issue because they use
thrift + ruby .
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#139 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD619mBcPLZ5DfTJbmAHLcAIXf5nTWIks5vRiSDgaJpZM4bTd0g>
.
|
|
here looks like it still uses thrift https://github.com/openzipkin/zipkin-ruby/blob/master/lib/zipkin-tracer/zipkin_kafka_tracer.rb#L37 if we switch this to a json list all good.. |
|
Oh, forgot about these guys. Yes, if the thrift library is added to the project which also contains zipkin-ruby library, then this adaptor loads it dynamically and uses it. So I guess we will need to support it or change this. It is scary because I've not kafka around, difficult to adhoc this. |
|
when I test kafka I use this...
https://github.com/openzipkin/docker-zipkin#kafka
…On Wed, Feb 27, 2019 at 5:03 PM Jordi Polo Carres ***@***.***> wrote:
Oh, forgot about these guys. Yes, if the thrift library is added to the
project which also contains zipkin-ruby library, then this adaptor loads it
dynamically and uses it. So I guess we will need to support it or change
this. It is scary because I've not kafka around, difficult to adhoc this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#139 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD618nI1MA3Uk0Gqe9bocSZcgeP456gks5vRknhgaJpZM4bTd0g>
.
|
|
got this error when requiring 'finagle-thrift': seems we lose the compatibility with thrift now. |
|
maybe different issue but converting Kafka to send same payload as http
probably good idea. then no games
…On Thu, Feb 28, 2019, 9:37 AM Yohei Kitamura ***@***.***> wrote:
got this error when requiring 'finagle-thrift':
Failure/Error: require 'finagle-thrift'
TypeError:
superclass mismatch for class NullTracer
seems we lose the compatibility with thrift now.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#139 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAD61yH5k1V6OBEnaAjr2IdUIpVa32XGks5vRzL1gaJpZM4bTd0g>
.
|
|
I think so, let's kill Thrift definitely, none of the maintainers have a decent setup to test it out and noone has stepped up to fill the role, I don't think it will be missed but if so, I'd gladly merge any PR related to it. |
|
it sounds like a separate story. I don't have a JRuby project, cannot test properly. |
lib/zipkin-tracer/config.rb
Outdated
| @@ -16,7 +16,7 @@ def initialize(app, config_hash) | |||
| # The name of the current service | |||
| @service_name = config[:service_name] | |||
| # The port where the current service is running | |||
| @service_port = config[:service_port] || DEFAULTS[:service_port] | |||
| @service_port = config[:service_port] | |||
There was a problem hiding this comment.
I dont thing we need this at all. This is only there for local and we are not sending it now, right?
README.md
Outdated
| @@ -18,7 +18,7 @@ use ZipkinTracer::RackHandler, config # config is optional | |||
| where `Rails.config.zipkin_tracer` or `config` is a hash that can contain the following keys: | |||
|
|
|||
| * `:service_name` **REQUIRED** - the name of the service being traced. There are two ways to configure this value. Either write the service name in the config file or set the "DOMAIN" environment variable (e.g. 'test-service.example.com' or 'test-service'). The environment variable takes precedence over the config file value. | |||
| * `:service_port` **REQUIRED** - the port of the service being traced (e.g. 80 or 443) | |||
| * `:service_port` - the port of the service being traced (e.g. 80 or 443) | |||
There was a problem hiding this comment.
can't we just delete this?
There was a problem hiding this comment.
updated in 29af492
|
Yes, let's finish this one and make the migration in some other story |
ykitamura-mdsol
changed the title
Changed the hardcoded value for 'sa' annotation to
true:openzipkin/zipkin#2414
Updated the ':service_port' configuration to be optionalRemoved the ':service_port' configuration: #137@adriancole @jcarres-mdsol