[filereceiver] support logs, traces, and compressed reads #22080

moh-osman3 · 2023-05-18T19:49:52Z

Description:
[PART 2] Breaking #22021 into 2 PRs
This PR completes the following:

supports traces and logs in the filereceiver
uses compressed reader to read any compressed data

For more context see part 1: #22079

Link to tracking Issue:
Related to #13626
Supports ongoing effort to record and replay telemetry data for experimental/research purposes

Testing:
Locally running otelcontribcol and writing to a file using all supported formats (json, json+compression, proto, proto+compression) and recovering original metrics using filereceiver

Documentation:

atoulme · 2023-05-30T18:55:42Z

receiver/filereceiver/file_reader.go

+// readLogLine reads the next line in the file, converting it into logs and
+// passing it to the the consumer member.
+func (fr fileReader) readLogLine(ctx context.Context) error {
+	line, err := fr.stringReader.ReadString('\n')


can you use a scanner here?

Thanks for the review! I believe a scanner would work here, but curious why that would be preferred over the string reader? Have never used a scanner in golang but glancing at bufio's scanner type and saw

Scanning stops unrecoverably at EOF, the first I/O error, or a token too large to fit in the buffer. When a scan stops, the reader may have advanced arbitrarily far past the last token. Programs that need more control over error handling or large tokens, or must run sequential scans on a reader, should use bufio.Reader instead

I'm wondering if this places a limitation on the size of file being read?

atoulme · 2023-05-30T18:56:03Z

receiver/filereceiver/file_reader.go

+		cr, _ := zstd.NewReader(file)
+		fr.stringReader = bufio.NewReader(cr)
+	} else { // no compression
+		fr.stringReader = bufio.NewReader(file)


Is there a reason to use a string reader?

No particular reason other than that was already what was being used by the receiver, so I am just extending this functionality to also be able to read chunks of data (instead of just lines). Is there a major drawback/concern with using the string reader you have in mind?

atoulme · 2023-05-30T18:57:00Z

receiver/filereceiver/file_reader.go

+		return err
+	}
+
+	dataBuffer := make([]byte, sz)


That's weird, what is this? Are we encoding each entry separately along with their size in the stream? Can we not use EOLs?

This is to accurately decode the method of writing the fileexporter uses for proto format. The filexporter currently already supports 2 distinct writing strategies. (1) Writing line by line or (2) writing chunks that are prepended with the size of the chunk (for proto format)

This added functionality now supports the second type of file writing in the exporter. We can't use EOL here because in this case the writer does not include any newlines so decoding would not be accurate

@atoulme Yes, the old existing code was weird!

djaglowski · 2023-05-30T19:18:30Z

I've suggested in the past that this receiver should use pkg/stanza/fileconsumer to track and read files. (See comment)

Given that this PR would duplicate additional functionality that is supported in pkg/stanza/fileconsumer shouldn't we first rebase onto that?

jmacd · 2023-06-07T18:06:02Z

@djaglowski The package that you referred to (a) sounds vendor specific -- are you willing to rename it? (b) does not have a README explaining why we should want it and what tradeoffs we might have to use it. Can you file an issue explaining why you'd like this package to be used in more detail? It looks like it is a useful package for production log reading, which is not what this receiver is aimed at doing.

As it stands, the current filereceiver is just a simple receiver designed to read a single file, which @moh-osman3 is not changing in this PR. I suggest we leave improvements in this package until there is a documented reason to do so. We are trying to resolve #13626, which is addressed by @moh-osman3's change. The OTel-Arrow project needs a way to record and replay files to be able to reproduce bugs, and our use-case is not helped by the feature you are requesting. Thank you!

djaglowski · 2023-06-07T19:43:57Z

@djaglowski The package that you referred to (a) sounds vendor specific -- are you willing to rename it? (b) does not have a README explaining why we should want it and what tradeoffs we might have to use it. Can you file an issue explaining why you'd like this package to be used in more detail? It looks like it is a useful package for production log reading, which is not what this receiver is aimed at doing.

As it stands, the current filereceiver is just a simple receiver designed to read a single file, which @moh-osman3 is not changing in this PR. I suggest we leave improvements in this package until there is a documented reason to do so. We are trying to resolve #13626, which is addressed by @moh-osman3's change. The OTel-Arrow project needs a way to record and replay files to be able to reproduce bugs, and our use-case is not helped by the feature you are requesting. Thank you!

The package exists in this repo and is used in generic components. What would you rename it to?

I think I must have misunderstood the proposal when I sponsored it. I understood that this would be a complement to the fileexporter, that it would be an accessible (if not ideal) way to transmit OTLP via file.

With that in mind, the main reason to use the fileconsumer package is because the common requirements for components that tail fails are likely to be very similar. We already have robust file tracking and tailing capabilities in the collector, which were specifically isolated so that we would not need to reimplement them in each file tailing component. It's not difficult to use the functionality - here an example of another component doing so in a few lines.

In any case, the reason I have brought this up in each consecutive PR is because I don't think we should continue down the road of redesigning configuration and reimplementing functionality that is likely to converge in the future, nor do I want to be responsible for maintaining a separate implementation.

Although I would still disagree with the approach, I'm happy to hand the decision off to any other @open-telemetry/collector-contrib-approvers who would like to take over sponsorship of the component.

moh-osman3 · 2023-06-11T19:00:54Z

Refactored this change to use the fileconsumer.PositionalScanner, and would appreciate some tips on using this package properly.

@djaglowski @atoulme would you mind taking another look at this change?

fatsheep9146 · 2023-06-12T06:55:44Z

receiver/filereceiver/config.go

+	// Format will specify the format of the file to be read.
+	// Currently support json and proto options.
+	FormatType string `mapstructure:"format"`
+	// type of compression algorithm used. Currently supports zstd.


type -> Type

fatsheep9146 · 2023-06-12T06:59:36Z

receiver/filereceiver/receiver.go

@@ -15,12 +15,19 @@ import (
 	"go.uber.org/zap"
 )

+type consumerType struct {


The name consumerType seemed a little confused.

Do you think is it better that we just directly use metricsConsumer, tracesConsumer , logsConsumer in the fileReceiver struct ?

fatsheep9146 · 2023-06-12T07:04:08Z

receiver/filereceiver/file_reader.go

+		timer:    timer,
+	}
+
+	if compression == compressionTypeZSTD {


This part seems redundant, maybe we can change as following?

fr.baseReader = file if compression == compressionTypeZSTD { cr, _ := zstd.NewReader(file) fr.baseReader = cr } if format == formatTypeProto { fr.scanner = fileconsumer.NewPositionalScanner(cr, 10 * 1024, int64(0), ScanChunks) } else { fr.scanner = fileconsumer.NewPositionalScanner(cr, 10 * 1024, int64(0), bufio.ScanLines) }

Thanks for the review! I may be changing how the fileconsumer package is used to address @djaglowski 's request, but will incorporate your feedback in my next change

djaglowski · 2023-06-12T15:06:16Z

@moh-osman3, I don't recommend using the PositionalScanner directly.

The Config and Manager should be used. Example here.

moh-osman3 · 2023-06-18T20:23:49Z

@moh-osman3, I don't recommend using the PositionalScanner directly.

The Config and Manager should be used. Example here.

@djaglowski Hmm I'm wondering how I could customize the split function used if I only use the Config and Manager? I see an option for SplitterConfig but struggling to get this to work. The only thing I can control is the MultilineConfig but don't think this will work for reading one of the formats of the fileexporter. I want a split function like https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/22080/files#diff-95d05abde2282d49c10dddc2bd412b09b2a59fedd7f081249123e8fbc4f7759fR41, which doesn't have a clear string delimiter (i.e. the delimiter is a uint32 indicating the number of bytes to read)

djaglowski · 2023-06-20T17:29:09Z

@moh-osman3, can you use Config.BuildWithSplitFunc?

github-actions · 2023-07-05T05:21:01Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2023-07-20T05:20:07Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

github-actions bot added the receiver/file label May 18, 2023

github-actions bot requested review from djaglowski and pmcollins May 18, 2023 19:50

moh-osman3 marked this pull request as ready for review May 18, 2023 19:55

moh-osman3 requested a review from a team May 18, 2023 19:55

github-actions bot assigned mx-psi May 18, 2023

This was referenced May 18, 2023

fileexporter/receiver: refactor fileexporter and read compressed json in filereceiver #22021

Closed

[fileexporter] refactor to use compressed writer in fileexporter #22079

Closed

jmacd approved these changes May 19, 2023

View reviewed changes

atoulme reviewed May 30, 2023

View reviewed changes

moh-osman3 added 5 commits June 11, 2023 14:54

filereceiver support logs, traces, and compressed reads

62762a1

chloggen

b6364dc

lint and add test file

c1b97ee

use fileconsumer positional scanner

25f6bbc

rebase

8cf6342

moh-osman3 force-pushed the mohosman/file-receiver-read-compressed-data branch from c584ece to 8cf6342 Compare June 11, 2023 18:59

remove unneeded stringreader test

bf56fff

fatsheep9146 reviewed Jun 12, 2023

View reviewed changes

moh-osman3 mentioned this pull request Jun 22, 2023

Add filereceiver component with updated functionality f5/otel-arrow-adapter#186

Merged

github-actions bot added the Stale label Jul 5, 2023

github-actions bot closed this Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[filereceiver] support logs, traces, and compressed reads #22080

[filereceiver] support logs, traces, and compressed reads #22080

moh-osman3 commented May 18, 2023 •

edited

Loading

atoulme May 30, 2023

moh-osman3 Jun 2, 2023

atoulme May 30, 2023

moh-osman3 Jun 2, 2023

atoulme May 30, 2023

moh-osman3 Jun 2, 2023

jmacd Jun 7, 2023

djaglowski commented May 30, 2023

jmacd commented Jun 7, 2023

djaglowski commented Jun 7, 2023

moh-osman3 commented Jun 11, 2023

fatsheep9146 Jun 12, 2023

fatsheep9146 Jun 12, 2023

fatsheep9146 Jun 12, 2023

moh-osman3 Jun 18, 2023

djaglowski commented Jun 12, 2023

moh-osman3 commented Jun 18, 2023

djaglowski commented Jun 20, 2023

github-actions bot commented Jul 5, 2023

github-actions bot commented Jul 20, 2023

[filereceiver] support logs, traces, and compressed reads #22080

[filereceiver] support logs, traces, and compressed reads #22080

Conversation

moh-osman3 commented May 18, 2023 • edited Loading

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

djaglowski commented May 30, 2023

jmacd commented Jun 7, 2023

djaglowski commented Jun 7, 2023

moh-osman3 commented Jun 11, 2023

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

djaglowski commented Jun 12, 2023

moh-osman3 commented Jun 18, 2023

djaglowski commented Jun 20, 2023

github-actions bot commented Jul 5, 2023

github-actions bot commented Jul 20, 2023

moh-osman3 commented May 18, 2023 •

edited

Loading