implement kafka-exporter #965
Conversation
|
/easycla |
|
/easycla |
There was a problem hiding this comment.
hi @naser-ayat!
are you willing to be a component owner for this going forward?
ideally we would like to have two component owners, maybe try posting in the #otel-java slack channel to see if anyone else is interested as well?
| this.timeoutInSeconds = timeoutInSeconds; | ||
| } | ||
|
|
||
| public KafkaSpanExporter( |
There was a problem hiding this comment.
Instead of adding more constructors you might want to opt in for using static factory methods or a builder like pattern. I would think static factory methods make the code much clearer. You could use some default names like create to replace the main constructor, and I've used things like basic, and with timeout etc etc as other options.
This gives the replacements names and makes it easier to work with.
But looking at all of the options you have a Builder might be a good option as well. probably with some default/required fields to start with
There was a problem hiding this comment.
A builder is added.
| @Override | ||
| public ExportTraceServiceRequest deserialize(String topic, byte[] data) { | ||
| if (Objects.isNull(data)) { | ||
| return ExportTraceServiceRequest.getDefaultInstance(); |
There was a problem hiding this comment.
Would you return an empty event? I think if it's not a valid response it should throw. Empty events hide the fact that nothing worthy of processing can be returned
There was a problem hiding this comment.
In the Deserialiser interface documentation, it states that the 'data' parameter, which represents serialized bytes, may be null. Implementations are advised to handle null by either returning a value or null instead of throwing an exception. That's why the exception is not thrown. I personally opted for returning an empty object to simplify dealing with null values in the callers. If you prefer to receive null values, please let me know.
There was a problem hiding this comment.
Returning a empty value creates a valid return value that in turn needs to be validated at the client side. This hides the fact that the the incoming event could not be deserialized.
In other words it transforms the result to a successful event with no content. Default null handling of client side libraries might be a better solution here. I understand that null doesn't seem like the best return value but based what you said about the docs this is the better return type here imo
This comment was marked as duplicate.
This comment was marked as duplicate.
We're currently discussing this at ING and will provide an update soon. Additionally, I've sent a message to the Slack channel to see if anyone else is interested. |
I'm interested in being a component owner. |
github-actions
bot
assigned anosek-an, breedx-splk, cyrille-leclerc, dehaansa, iNikem, kenfinnigan, laurit, mateuszrzeszutek, Mrod1598 and oertl
naser-ayat
force-pushed
the
kafka-exporter
branch
from
ee2d8e4
to
125906d
Compare
| @@ -58,3 +58,6 @@ components: | |||
| - trask | |||
| static-instrumenter: | |||
| - anosek-an | |||
| kafka-exporter: | |||
| - spockz | |||
| @@ -58,3 +58,6 @@ components: | |||
| - trask | |||
| static-instrumenter: | |||
| - anosek-an | |||
| kafka-exporter: | |||
| - spockz | |||
| - vincentfree | |||
There was a problem hiding this comment.
@naser-ayat did you want to be a component owner as well?
There was a problem hiding this comment.
Two component owners should be enough. Thanks for the suggestion.
There was a problem hiding this comment.
@vincentfree Could you please approve the PR?
There was a problem hiding this comment.
it would be great to get approvals from both of the component owners
There was a problem hiding this comment.
@vincentfree can you approve via github? thx!
There was a problem hiding this comment.
@spockz ^^ can you approve via github also?
There was a problem hiding this comment.
Done, sorry was going to do an actual review but I did that before so that's fine now. finally came around to click that button :)
Description:
The
kafka-exportermodule is added. This module containsKafkaSpanExporter(an implementation of theSpanExporterinterface) that can be used for sendingSpanDatato Kafka.Existing Issue(s):
Solves #145
Testing:
The module contains both unit tests and an integration test.
Documentation:
A README file documents the usage