Adding Spring properties integration to handle configuration #5
Conversation
|
Hi @Diagoras , thanks a lot for the PR, any contribution is welcome. Cheers. |
|
@matelang Absolutely, no worries. Thanks for supporting this integration! |
| /** | ||
| * Report traces to the configured Datadog Agent. | ||
| * | ||
| * @param host (See DDApi.DEFAULT_HOSTNAME) | ||
| * @param port (See DDApi.DEFAULT_PORT) | ||
| */ | ||
| public DatadogReporter(String host, int port) { | ||
| this(host, port, new LinkedBlockingQueue<>()); | ||
| public DatadogReporter(String host, int port, int messageTimeout) { |
There was a problem hiding this comment.
Maybe it would be better to pass the entire DatadogProperties class to the constructor. Also have a default constructor that creates a DatadogProperties object with the default values.
There was a problem hiding this comment.
@ambrusadrianz That would be nice, but unfortunately would require establishing a dependency from this module to "zipkin-datadog-reporter-autoconfigure". I think that's the reason why the default Zipkin reporter for Spring Sleuth also implements the constructor this way.
|
Hi @Diagoras, Thanks for the PR. I left some comments, just some minor, nitpicks otherwise looks good. Thanks a lot for your PR. Cheers. |
|
@matelang @ambrusadrianz Anything else needed, or do you think this LGTM? |
This change allows external configuration of the Datadog reporter, with most of the properties and their namespaces hijacked from the Zipkin reporter's in Spring Sleuth.
BTW, how would you feel about adding Issues to this GitHub repo? Might make it easier to propose and discuss changes before opening PRs.