Add an HTTP Sender (#165) #171

mdouaihy · 2019-09-10T11:38:37Z

Support an HTTP Sender that allows to connect directly to a Collector.
For instance, authentication is not supported.

Signed-off-by: FR-MUREX-COM\mchaikhadouaihy mehrez.douaihy@gmail.com

Which problem is this PR solving?

Missing HTTP sender to connect directly to the jaeger collector

Short description of the changes

Add the HTTP sender
refactor UDPTransport to ThriftTransport
refactor UDPClient to UDPSender

AppVeyorBot · 2019-09-10T12:11:59Z

✅ Build jaeger-client-cpp 30 completed (commit 0d5d404a6f by @mdouaihy)

mdouaihy · 2019-09-10T16:20:41Z

@yurishkuro

yurishkuro

It would be good to add an integration test for HTTP sender, similar to how it's done in the Java client:

README.md

yurishkuro · 2019-09-14T16:59:01Z

src/jaegertracing/ThriftTransport.cpp

+
+}  // anonymous namespace
+
+ThriftTransport::ThriftTransport(const net::IPAddress& ip,


I am not sure about this renaming. Thrift is an encoding, not a transport. UDP vs. HTTP are the transports (although we call the "senders").

I agree, Thrift is just the encoding.

Knowing that it creates a thrift::Batch from a span and sends it to a sender to be sent. What do you suggest as naming? ThriftSender?

I renamed it to ThriftSender because it takes and Span and transform it to ThriftSpan and flush it via a UDP/Http sender.

What do you think?

yurishkuro · 2019-09-14T17:03:09Z

src/jaegertracing/ThriftTransport.cpp

+{
+}
+
+ThriftTransport::ThriftTransport(const net::URI& endpoint,


This looks like an unnecessary abstraction leak of the sender implementation into this class. Why not just accept sender as a ctor argument?

Indeed. You're right. The config will now build the actual sender and pass it as argument to the ctor.

yurishkuro · 2019-09-14T17:15:53Z

src/jaegertracing/utils/HttpSender.h

+        uint8_t* data = nullptr;
+        uint32_t size = 0;
+        _buffer->getBuffer(&data, &size);
+        if (static_cast<int>(size) > _maxPacketSize) {


a) how is _maxPacketSize defined for HTTP?
b) does it matter if it's exceeded?

Also, I haven't looked into Sender implementation here, but in other clients (e.g. Go), senders simply don't reach the state where a buffer is overflowed and spans must be dropped, because sender would flush previous spans if append(span) exceeds msg size. But again, this only makes sense for UDP because of fragmentation, it's not very relevant to HTTP (i.e. +/- extra span, who cares)

Indeed, this should not happen.

As for the max value, it's the same as the Java implementation.

This limit is now used only to know when to flush.

AppVeyorBot · 2019-09-15T21:46:32Z

✅ Build jaeger-client-cpp 33 completed (commit 318c05a84f by @mdouaihy)

mdouaihy · 2019-09-15T21:51:00Z

Integration test added.

AppVeyorBot · 2019-09-15T22:17:36Z

✅ Build jaeger-client-cpp 34 completed (commit f366a79b94 by @mdouaihy)

yurishkuro · 2019-09-16T17:59:54Z

src/jaegertracing/Transport.cpp

@@ -14,4 +14,4 @@
 * limitations under the License.
 */

-#include "jaegertracing/Transport.h"
+#include "jaegertracing/Sender.h"


what's the purpose of this Transport.cpp?

Oh, this file was not renamed automatically when I renamed the class. should be fixed.

src/jaegertracing/reporters/Config.h

AppVeyorBot · 2019-09-16T21:43:34Z

✅ Build jaeger-client-cpp 36 completed (commit b3863274ab by @mdouaihy)

AppVeyorBot · 2019-09-16T22:11:04Z

✅ Build jaeger-client-cpp 37 completed (commit 0a61db7674 by @mdouaihy)

mdouaihy · 2019-09-17T19:57:52Z

@yurishkuro. any additional remarks on this PR?

yurishkuro · 2019-09-25T16:57:27Z

@mdouaihy could you resolve the conflicts / merge master?

Support an HTTP Sender that allows to connect directly to a Collector. For instance, authentication is not supported. Signed-off-by: FR-MUREX-COM\mchaikhadouaihy <mehrez.douaihy@gmail.com>

* Rename ThriftTransporter to ThriftSender * Add Integration Test for http Signed-off-by: FR-MUREX-COM\mchaikhadouaihy <mehrez.douaihy@gmail.com>

Signed-off-by: FR-MUREX-COM\mchaikhadouaihy <mehrez.douaihy@gmail.com>

AppVeyorBot · 2019-09-27T00:04:38Z

✅ Build jaeger-client-cpp 40 completed (commit 67c94429f1 by @mdouaihy)

mdouaihy · 2019-09-27T00:06:22Z

Hi @yurishkuro, I rebased the PR to the latest commit.

mdouaihy · 2019-09-29T16:12:33Z

hi @yurishkuro, anything missing to merge this PR?

yurishkuro · 2019-09-29T18:14:46Z

@mdouaihy the readme only seems to mention a file-based configuration. Does this support env var configuration like JAEGER_ENDPOINT, per https://www.jaegertracing.io/docs/1.14/client-features/ ?

mdouaihy · 2019-09-29T19:07:42Z

@yurishkuro, this implementation does not support configuration via the env variable, neither for the agent nor for the collector.

We can work on supporting that if you think it's important.

yurishkuro · 2019-09-29T20:07:29Z

Not sure how critical it is if nobody asked so far. I booked a ticket anyway: #178

yurishkuro reviewed Sep 14, 2019

View reviewed changes

mdouaihy force-pushed the httpsender branch from a077dd8 to 778793c Compare September 15, 2019 21:51

yurishkuro reviewed Sep 16, 2019

View reviewed changes

mdouaihy force-pushed the httpsender branch from a58a55e to 7e7bed6 Compare September 16, 2019 21:18

yurishkuro approved these changes Sep 25, 2019

View reviewed changes

mdouaihy added 3 commits September 27, 2019 01:28

Add an HTTP Sender (jaegertracing#165)

ea40f20

Support an HTTP Sender that allows to connect directly to a Collector. For instance, authentication is not supported. Signed-off-by: FR-MUREX-COM\mchaikhadouaihy <mehrez.douaihy@gmail.com>

* Rename Http/UDP senders to Http/UDP Transporters

bfc01a0

* Rename ThriftTransporter to ThriftSender * Add Integration Test for http Signed-off-by: FR-MUREX-COM\mchaikhadouaihy <mehrez.douaihy@gmail.com>

Make HTTP capital to be inline with UDP.

53a2f88

Signed-off-by: FR-MUREX-COM\mchaikhadouaihy <mehrez.douaihy@gmail.com>

mdouaihy force-pushed the httpsender branch from 7e7bed6 to 53a2f88 Compare September 26, 2019 23:37

yurishkuro merged commit 4ee8a66 into jaegertracing:master Sep 29, 2019

mdouaihy deleted the httpsender branch September 29, 2019 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an HTTP Sender (#165) #171

Add an HTTP Sender (#165) #171

mdouaihy commented Sep 10, 2019

AppVeyorBot commented Sep 10, 2019

mdouaihy commented Sep 10, 2019

yurishkuro left a comment

yurishkuro Sep 14, 2019

mdouaihy Sep 14, 2019

mdouaihy Sep 15, 2019

yurishkuro Sep 14, 2019

mdouaihy Sep 15, 2019

yurishkuro Sep 14, 2019

mdouaihy Sep 14, 2019 •

edited

Loading

AppVeyorBot commented Sep 15, 2019

mdouaihy commented Sep 15, 2019

AppVeyorBot commented Sep 15, 2019

yurishkuro Sep 16, 2019

mdouaihy Sep 16, 2019

AppVeyorBot commented Sep 16, 2019

AppVeyorBot commented Sep 16, 2019

mdouaihy commented Sep 17, 2019

yurishkuro commented Sep 25, 2019

AppVeyorBot commented Sep 27, 2019

mdouaihy commented Sep 27, 2019

mdouaihy commented Sep 29, 2019

yurishkuro commented Sep 29, 2019

mdouaihy commented Sep 29, 2019

yurishkuro commented Sep 29, 2019


		} // anonymous namespace

		ThriftTransport::ThriftTransport(const net::IPAddress& ip,

Add an HTTP Sender (#165) #171

Add an HTTP Sender (#165) #171

Conversation

mdouaihy commented Sep 10, 2019

Which problem is this PR solving?

Short description of the changes

AppVeyorBot commented Sep 10, 2019

mdouaihy commented Sep 10, 2019

yurishkuro 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

Choose a reason for hiding this comment

mdouaihy Sep 14, 2019 • edited Loading

Choose a reason for hiding this comment

AppVeyorBot commented Sep 15, 2019

mdouaihy commented Sep 15, 2019

AppVeyorBot commented Sep 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Sep 16, 2019

AppVeyorBot commented Sep 16, 2019

mdouaihy commented Sep 17, 2019

yurishkuro commented Sep 25, 2019

AppVeyorBot commented Sep 27, 2019

mdouaihy commented Sep 27, 2019

mdouaihy commented Sep 29, 2019

yurishkuro commented Sep 29, 2019

mdouaihy commented Sep 29, 2019

yurishkuro commented Sep 29, 2019

mdouaihy Sep 14, 2019 •

edited

Loading