How to create an instance of zipkin2.finagle.kafka.KafkaZipkinTracer with Kafka SSL configuration?

[Tomasz Michnik]

Hi,

I have a problem with creating KafkaZipkinTracer. 

ZipkinTracer.Config allows to provide: bootstrapServers, topic, initialSampleRate. 

Unfortunately it is not enough for our purposes. We need a possibility to provide more detailed  information as Kafka configuration:  SSL settings, acks, security.protocol, spans' encoding.

zipkin2.finagle.kafka.KafkaZipkinTracer contains the following with public access:

public KafkaZipkinTracer() {

    this(Config.builder().build(), DefaultStatsReceiver$.MODULE$.get().scope("zipkin.kafka"));

}

public static KafkaZipkinTracer create(String bootstrapServers, StatsReceiver stats) {

    return new KafkaZipkinTracer(Config.builder().bootstrapServers(bootstrapServers).build(), stats);

}

public static KafkaZipkinTracer create(Config config, StatsReceiver stats) {

    return new KafkaZipkinTracer(config, stats);

}

On    https://github.com/openzipkin/zipkin-finagle/blob/master/kafka/src/test/java/zipkin2/finagle/kafka/KafkaZipkinTracerIntegrationTest.java 

I found some example:

tracer = new KafkaZipkinTracer(KafkaSender.newBuilder() .bootstrapServers(config.bootstrapServers())         .topic(config.topic())         .overrides(overrides)         .build(), config, stats);

which would suit our needs if this constructor had a public access.  Unfortunately from some reason it is of default scope.

 KafkaZipkinTracer(KafkaSender kafka, Config config, StatsReceiver stats)

As an interim solution I created a factory method in zipkin2.finagle.kafka. However it is an ugly solution which I'd like to remove.

[Tomasz Michnik]

In my humble opinion there should be provided  either a public factory method or a public constructor: which accepts: KafkaSender, Config and StatsReceiver.

What do you think guys?

[Adrian Cole]

you are the first to ask for this ever, probably because people often use flags for config and service loader requires the public ctor.

Another option btw is to just extend ZipkinTracer and make your own thing. If you decide to approach this as a pull request, note that the sender has to be closed externally. Meaning we can't assume that we should close something we didn't open. Anyway if I were you I'd probably just extend ZipkinTracer as it is quite more straightforward to make whatever you like that way.

--

---
You received this message because you are subscribed to the Google Groups "zipkin-user" group.
To unsubscribe from this group and stop receiving emails from it, send an email to zipkin-user...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/6e63b784-17aa-427f-b377-aeabe06dec46%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Tomasz Michnik]

Hi,

I guess that people do not need SSL  configuration for Kafka.

Unfortunately, in our case it is compulsory. 

tracer = new KafkaZipkinTracer(KafkaSender.newBuilder()
.bootstrapServers(config.bootstrapServers())
        .topic(config.topic())
        .overrides(overrides

)
        .encoding(encoding(tracingConfig))
        .build(), config, stats);

On the code sample  overrides is just a Map with properties used by Kafka.

ie. 

  val kafkaProperties = new java.util.HashMap[String, Any]()

    kafkaProperties.put(SslConfigs.SSL_KEYSTORE_LOCATION_CONFIG,  /* something */)

    kafkaProperties.put(SslConfigs.SSL_KEYSTORE_PASSWORD_CONFIG, /* something */)

    kafkaProperties.put(SslConfigs.SSL_KEY_PASSWORD_CONFIG, /* something */ )

    kafkaProperties.put(SslConfigs.SSL_TRUSTSTORE_LOCATION_CONFIG, /* something */)

    kafkaProperties.put(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG, /* something */)

    kafkaProperties.put(SslConfigs.SSL_KEYSTORE_TYPE_CONFIG, /* something */)

    kafkaProperties.put(SslConfigs.SSL_TRUSTSTORE_TYPE_CONFIG, /* something */)

    kafkaProperties.put(SslConfigs.SSL_ENABLED_PROTOCOLS_CONFIG, /* something */)

    kafkaProperties.put(ProducerConfig.ACKS_CONFIG, /* something */)

    kafkaProperties.put(CommonClientConfigs.SECURITY_PROTOCOL_CONFIG, /* something */)

There is also encoding but kafka properties are the most important.

Regarding Flags, I do need see how it could help us to pass  Kafka properties to KafkaZipkinTracer  (except bootstrapServers, topic, initialSampleRate). 

Extending ZipkinTracer  is not an option for me as well. I should basically copy KafkaZipkinTracer content exposing KafkaZipkinTracer(KafkaSender kafka, Config config, StatsReceiver stats). 

From the clean code perspective it is not a better solution then my ugly workaround.

Is it really a big problem to let users to pass KafkaSender  to KafkaZipkinTracer either with a create method or with a constructor?

To unsubscribe from this group and stop receiving emails from it, send an email to zipki...@googlegroups.com.

[Adrian Cole]

I am not sure why extending zipkin tracer is not an option for you. The only reason the wrapper exists at all is for managing flags which you mention you dont use anyway. literally you are the only person to ask for change and our culture is not towards making change for a single person.

If you insist on the only acceptable way to proceed is to use this type...

a public static factory method is an option. what the doc should say is that the supplied sender will be closed when the tracer is.

To unsubscribe from this group and stop receiving emails from it, send an email to zipkin-user...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/e8170daa-9eae-4fda-a186-e3ce34bad011%40googlegroups.com.

[Adrian Cole]

I made this type which should satisfy the requirements. As code change happens on github, we need feedback before any of this ends up in a release. If feedback is quick, smooth and kind, releasing is easy

https://github.com/openzipkin/zipkin-finagle/pull/69

To unsubscribe from this group and stop receiving emails from it, send an email to zipkin-user+unsubscribe@googlegroups.com.

[Tomasz Michnik]

Hi,

I can see that zipkin-finagle 2.1.0 has been already released.

There is a new feature which allows to build zipkin2.finagle.ZipkinTracer "manually" with a builder.

That might address our 2 problems: access to zipkin  logging through zipkin2.reporter.ReporterMetrics custom implementation and Kafka SSL configuration.

Unfortunately, in ZipkinTracer Javadoc  I found a statement    which worries me a bit.

"You must close the supplied sender externally, after this instance is closed." I wonder how can I close sender  as I only pass it to  Zipkin tracer and then  the tracer to finagle client and thats it.

I do not influence finagle anyhow  how and when to close a tracer.

Tom

[Adrian Cole]

good question though.. I don't know how finagle shuts things down either. maybe it doesn't automatically shut things down? in such case it is moot. Regardless, you can add a shutdown hook and that will work.

Runtime.getRuntime().addShutdownHook(new Thread(() -> {

try {

sender.close(); // Release any network resources used to send spans

} catch (IOException e) {

Logger.getAnonymousLogger().warning("error closing trace sender: " + e.getMessage());

}

}));

To unsubscribe from this group and stop receiving emails from it, send an email to zipkin-user...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/9d4a0742-9d9c-4307-9448-4b2265c2e507%40googlegroups.com.

[Tomasz Michnik]

Probably it  won’t work for us as we still have many users deploying their apps on JBoss EAP.

Is there a chance to extend ZipkinTracer.Builder and add a method which takes some functional interface (callback) which would be called by  ZipkinTrace while closing?

Another option, maybe less elegant but suffice would be making HttpZipkinTracer and KafkaZipkinTracer non final and exposing a constructor or a relevant factory method in ZipkinTracer which takes : Reporter<Span> reporter, Config config, StatsReceiver stats.

To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/9d4a0742-9d9c-4307-9448-4b2265c2e507%40googlegroups.com.

[Adrian Cole]

if it doesn't work for you, doesn't that imply you also have a problem with finagle's lifecycle? if you are using jboss, don't you have other mechanisms available to register lifecycle (ala spring bean's destroy-on-close type of stuff)

To unsubscribe from this group and stop receiving emails from it, send an email to zipkin-user...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/6a4d8df7-1d25-4b9e-80a0-1a1530a7ecd9%40googlegroups.com.

[Adrian Cole]

anyway if you would like some advice about jboss, please share how you are managing finagle. Something must be integrating it as CDI dependency or something like that especially if you are programming it. CDI has mechanisms like PostDestroy and you can call close in sequence using that for example. We don't build in things like spring, guice, cdi etc into our core libraries as all of these things have mechanisms to deal with lifecycle. Let's solve the problem at the right abstraction. here's a demo you can fork to show how you are jbossing if you need second eyes. https://github.com/openzipkin/zipkin-finagle-example

[Adrian Cole]

lol those docs drifted. the method signature no longer accepts a
sender. it closes the reporter given to it! I'll update the docs.

[Adrian Cole]

here's a revision, which shows a dirty hack you can use. I really have
no idea how one can get jboss to use something then not be able to
shut it down. If you have an example of that, please paste a gist or
something for no other reason than curiousity

https://github.com/openzipkin/zipkin-finagle/pull/73

[Tomasz Michnik]

Which method you are talking about?

W dniu piątek, 9 sierpnia 2019 12:33:20 UTC+2 użytkownik Adrian Cole napisał:

lol those docs drifted. the method signature no longer accepts a
sender. it closes the reporter given to it! I'll update the docs.

>>> To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/6a4d8df7-1d25-4b9e-80a0-1a1530a7ecd9%40googlegroups.com.