Fluent Forward Receiver #1173

keitwb · 2020-06-24T00:38:09Z

This can receive logs in the Fluent Forward Protocol
via either TCP or Unix sockets. It supports all of the required features of
the spec.

It currently does not support the optional handshake aspect of the protocol,
but will acknowledge chunk options per the spec.

All log entries received within a short time window of each other will be put
into the same ResourceLog instance before being emitted to the next consumer in
the pipline. The Resource object is currently not populated in any way.

tigrannajaryan

Good start, thanks.
My primary concern is about the usage of Append vs Resize and performance implications of that.

receiver/fluentforwardreceiver/config.go

cmd/pdatagen/internal/base_structs.go

tigrannajaryan · 2020-06-24T15:01:29Z

receiver/fluentforwardreceiver/conversion.go

+		switch ev := event.(type) {
+		case *protocol.MessageEvent:
+			lr := convertMessageEvent(ev)
+			logSlice.Append(lr)


Using Append which calls Resize may be slow if we have large batches. Worth profiling for large batches (e.g. for thousands of events).
Alternatively, is it possible to use Resize upfront? Initially we know that the size is going to be in the ballpark of len(events), right?
If you use Resize you can't use Append anymore, you will need to use At() and track the index yourself.

I changed Append to be as efficient as normal slice append is in Go.

It is good that Append is fast however convertMessageEvent still returns a pointer, so we make a separate allocation per log record. If we call Resize before the for loop there will be a single allocation for the entire batch.

Again, I do not think having an Append is a good idea generally for slice creation. It allows/encourages to write code that makes more allocations than the Resize version which forces to combine the allocations. I may be missing something, please correct me if I am wrong.

We could merge this PR as is since Append is likely not going to affect performance a lot in this case but I am worried that the Append-based approach will bite us in the future and we will not be able to remove Append due to compatibility reasons. Because of that unless you have other reasons to keep Append I think it is best to remove it although its impact in this particular case may be negligible.

Append is now only used where the size of events cannot be determined upfront. Without Append in those cases, you would have to parse all of the events to some big slice of intermediate data structures and then do the Resize at the end, which is almost certainly going to result in worse performance due to all of the allocations of the intermediate data structures that would get thrown away.

receiver/fluentforwardreceiver/conversion.go

tigrannajaryan · 2020-06-24T15:17:40Z

receiver/fluentforwardreceiver/conversion.go

+}
+
+func convertMessageEvent(me *protocol.MessageEvent) *pdata.LogRecord {
+	lr := pdata.NewLogRecord()


NewLogRecord is slow since it does an allocation.
It is faster to accept as a function parameter the LogRecord that is returned by At() in the caller and populate it here instead of creating, returning it and Append-ing it. This is the intended way to use the API. The current usage will result in suboptimal performance and it is enabled by Append, so I would drop Append to avoid provoking such usage.

The new implementation of Append will be roughly the same since it will only call NewLogRecord once now.

receiver/fluentforwardreceiver/protocol/types.go

receiver/fluentforwardreceiver/receiver.go

receiver/fluentforwardreceiver/server.go

codecov · 2020-06-30T19:24:44Z

Codecov Report

Merging #1173 into master will decrease coverage by 0.43%.
The diff coverage is 76.95%.

@@            Coverage Diff             @@
##           master    #1173      +/-   ##
==========================================
- Coverage   90.50%   90.07%   -0.44%     
==========================================
  Files         219      231      +12     
  Lines       15521    16033     +512     
==========================================
+ Hits        14047    14441     +394     
- Misses       1060     1136      +76     
- Partials      414      456      +42

Impacted Files	Coverage Δ
exporter/exportertest/sink_exporter.go	`96.55% <0.00%> (-3.45%)`	⬇️
receiver/fluentforwardreceiver/conversion.go	`63.39% <63.39%> (ø)`
receiver/fluentforwardreceiver/testdata/parse.go	`68.42% <68.42%> (ø)`
receiver/fluentforwardreceiver/server.go	`79.00% <79.00%> (ø)`
receiver/fluentforwardreceiver/heartbeat.go	`80.00% <80.00%> (ø)`
receiver/fluentforwardreceiver/receiver.go	`94.11% <94.11%> (ø)`
internal/data/common.go	`100.00% <100.00%> (ø)`
receiver/fluentforwardreceiver/ack.go	`100.00% <100.00%> (ø)`
receiver/fluentforwardreceiver/collector.go	`100.00% <100.00%> (ø)`
receiver/fluentforwardreceiver/factory.go	`100.00% <100.00%> (ø)`
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cc89783...3d46580. Read the comment docs.

tigrannajaryan · 2020-07-07T23:53:59Z

internal/data/opentelemetry-proto-gen/common/v1/helpers.go

@@ -0,0 +1,25 @@
+package v1


I think opentelemetry-proto-gen directory contains generated files only. Can we move this file somewhere else?

tigrannajaryan · 2020-07-07T23:54:32Z

receiver/fluentforwardreceiver/config.go

+
+	// Configures the receiver to use TLS.
+	// The default value is nil, which will cause the receiver to not use TLS.
+	// TLSCredentials *configtls.TLSSetting `mapstructure:"tls, omitempty"`


Is this going to be uncommented?

No, removing it for now. We can add back later if people really want TLS support but given the target use case of localhost traffic it seems excessive.

tigrannajaryan · 2020-07-07T23:57:05Z

receiver/fluentforwardreceiver/conversion.go

+		switch ev := event.(type) {
+		case *protocol.MessageEvent:
+			lr := convertMessageEvent(ev)
+			logSlice.Append(lr)


It is good that Append is fast however convertMessageEvent still returns a pointer, so we make a separate allocation per log record. If we call Resize before the for loop there will be a single allocation for the entire batch.

Again, I do not think having an Append is a good idea generally for slice creation. It allows/encourages to write code that makes more allocations than the Resize version which forces to combine the allocations. I may be missing something, please correct me if I am wrong.

We could merge this PR as is since Append is likely not going to affect performance a lot in this case but I am worried that the Append-based approach will bite us in the future and we will not be able to remove Append due to compatibility reasons. Because of that unless you have other reasons to keep Append I think it is best to remove it although its impact in this particular case may be negligible.

tigrannajaryan

LGTM, except the comment about tests.

tigrannajaryan · 2020-07-17T20:06:28Z

receiver/fluentforwardreceiver/conversion_test.go

+	otlpcommon "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/common/v1"
+	logsproto "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/logs/v1"
+	otlpresource "go.opentelemetry.io/collector/internal/data/opentelemetry-proto-gen/resource/v1"


It is not advised to use otlp structs directly in component code or int component tests. OTLP structs may change over time (e.g. by using custom gogoproto data types). We provide wrappers for the purpose of encapsulating and isolating component code from such changes.
I highly advise to rewrite these tests not to use otlp structs at all. Work with pdata only.

Ok, I added a test util to easily construct logs in a declarative manner using the pdata methods only.

receiver/fluentforwardreceiver/factory.go

keitwb · 2020-07-20T15:51:51Z

Also @tigrannajaryan, wherever the size of the log records can be predetermined I got rid of the use of Append, but it is still in a few places where the event count cannot be determined upfront due to the msgpack protocol (e.g. packed forward events).

This can receive logs in the [Fluent Forward Protocol](https://github.com/fluent/fluentd/wiki/Forward-Protocol-Specification-v1) via either TCP or Unix sockets. It supports all of the required features of the spec. It currently does not support the optional handshake aspect of the protocol, but will acknowledge chunk options per the spec. All log entries received within a short time window of each other will be put into the same ResourceLog instance before being emitted to the next consumer in the pipline. The Resource object is currently not populated in any way.

tigrannajaryan

LGTM, great job.
Do you feel that more coverage is needed anywhere?

bogdandrutu · 2020-07-22T14:55:02Z

@tigrannajaryan please make sure new code is added with the right coverage. Was a hard effort for me to improve it to this level

* [demo] Increase Kafka memory limit and bump charts * Apply suggestion * Update examples

tigrannajaryan reviewed Jun 24, 2020

View reviewed changes

tigrannajaryan self-assigned this Jun 29, 2020

tigrannajaryan reviewed Jul 8, 2020

View reviewed changes

keitwb marked this pull request as ready for review July 13, 2020 15:11

keitwb requested review from bogdandrutu, dmitryax, flands, james-bebbington, owais, pjanotti and rghetia as code owners July 13, 2020 15:11

tigrannajaryan reviewed Jul 17, 2020

View reviewed changes

tigrannajaryan approved these changes Jul 22, 2020

View reviewed changes

tigrannajaryan merged commit d38076e into open-telemetry:master Jul 22, 2020

keitwb deleted the fluentbit-forward-receiver branch July 22, 2020 15:14

tigrannajaryan mentioned this pull request Jul 27, 2020

Implement Fluentd Forward Protocol v1 receiver in Collector. #1016

Closed

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

Skip build/test CI for PRs with only doc changes (open-telemetry#1173)

af445b8

swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024

[demo] Increase Kafka memory limit and bump charts (open-telemetry#1173)

2a5f647

* [demo] Increase Kafka memory limit and bump charts * Apply suggestion * Update examples

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fluent Forward Receiver #1173

Fluent Forward Receiver #1173

keitwb commented Jun 24, 2020 •

edited

Loading

tigrannajaryan left a comment

tigrannajaryan Jun 24, 2020

keitwb Jun 30, 2020

tigrannajaryan Jul 7, 2020

keitwb Jul 20, 2020

tigrannajaryan Jun 24, 2020

keitwb Jun 30, 2020

codecov bot commented Jun 30, 2020 •

edited

Loading

tigrannajaryan Jul 7, 2020

tigrannajaryan Jul 7, 2020

keitwb Jul 13, 2020

tigrannajaryan Jul 7, 2020

tigrannajaryan left a comment

tigrannajaryan Jul 17, 2020

keitwb Jul 20, 2020

keitwb commented Jul 20, 2020

tigrannajaryan left a comment

bogdandrutu commented Jul 22, 2020

Fluent Forward Receiver #1173

Fluent Forward Receiver #1173

Conversation

keitwb commented Jun 24, 2020 • edited Loading

tigrannajaryan 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

codecov bot commented Jun 30, 2020 • edited Loading

Codecov Report

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

tigrannajaryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keitwb commented Jul 20, 2020

tigrannajaryan left a comment

Choose a reason for hiding this comment

bogdandrutu commented Jul 22, 2020

keitwb commented Jun 24, 2020 •

edited

Loading

codecov bot commented Jun 30, 2020 •

edited

Loading