Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[exporter/file] add append mode #31369

Merged
merged 1 commit into from
Mar 6, 2024
Merged

[exporter/file] add append mode #31369

merged 1 commit into from
Mar 6, 2024

Conversation

OverOrion
Copy link
Contributor

@OverOrion OverOrion commented Feb 21, 2024 •
edited
Loading

Description:

This adds a new option for configuring the append / truncate behavior of the fileexporter.

Link to tracking Issue: #31364

Testing: Added TestAppend unit test and manually tested using telemetrygen and the following configuration:

receivers:
  otlp:
    protocols:
      grpc:
        endpoint: 0.0.0.0:4317

exporters:
  file:
    path: ./receiver_output_append.log
    append: true
service:
  telemetry:
    metrics:
      level: detailed
      address: 0.0.0.0:9998
  pipelines:
    logs:
      receivers: [otlp]
      exporters: [file]

Documentation:

TODO:

  • add documentation once we reached agreement regarding implementation / naming

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Feb 21, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: OverOrion / name: Szilard Parrag (a7b43a5)

@github-actions github-actions bot requested a review from atingchen February 21, 2024 18:12
@OverOrion OverOrion force-pushed the feat_exporter/file_append_mode branch from bf85197 to 05e0920 Compare February 22, 2024 08:57
@OverOrion OverOrion mentioned this pull request Feb 22, 2024
Closed
@OverOrion OverOrion marked this pull request as ready for review February 22, 2024 15:56
@OverOrion OverOrion requested review from a team and codeboten February 22, 2024 15:56
@OverOrion OverOrion force-pushed the feat_exporter/file_append_mode branch from 05e0920 to 6cd7b8f Compare February 23, 2024 07:44
@OverOrion OverOrion requested a review from atingchen February 23, 2024 07:45
@OverOrion OverOrion changed the title WIP: [exporter/file] add append mode [exporter/file] add append mode Feb 23, 2024
@OverOrion OverOrion force-pushed the feat_exporter/file_append_mode branch from 6cd7b8f to 4a50bab Compare February 26, 2024 13:17
@andrzej-stencel
Copy link
Member

Does the append mode work together with compression? If no, this should be mentioned in the documentation. If yes (can it?), we should have unit tests for this.

@andrzej-stencel
Copy link
Member

Does the append mode work together with compression? If no, this should be mentioned in the documentation. If yes (can it?), we should have unit tests for this.

Perhaps just document in the README that the append option currently only works when compression is disabled, and that this may change in the future.

@OverOrion OverOrion force-pushed the feat_exporter/file_append_mode branch 3 times, most recently from 1e9ecb8 to f7b2256 Compare March 1, 2024 07:30
@OverOrion OverOrion force-pushed the feat_exporter/file_append_mode branch from f7b2256 to 36204dd Compare March 1, 2024 09:18
andrzej-stencel
andrzej-stencel reviewed Mar 1, 2024
View reviewed changes
exporter/fileexporter/file_exporter_test.go
@@ -673,3 +674,86 @@ func TestFlushing(t *testing.T) {
assert.EqualValues(t, b, bbuf.Bytes())
assert.NoError(t, fe.Shutdown(ctx))
}

func TestAppend(t *testing.T) {
Copy link
Member

@andrzej-stencel andrzej-stencel Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is a bit too low level to my taste, using fileWriter directly. I think it's because it was based on the TestFlushing test. Perhaps better to write this test more like the TestFileTracesExporter and similar tests are written:

  1. create newFileExporter()
  2. generate traces
  3. write telemetry with consumeTraces
  4. check that traces written (check number of lines with JSON or sth)
  5. create a new instance of newFileExporter
  6. consumeTraces
  7. check the cumulative number of lines in file, or that the size of file has increased from point 4.

@OverOrion
[exporter/file] add append mode
a7b43a5 
Signed-off-by: Szilard Parrag <[email protected]>
@OverOrion OverOrion force-pushed the feat_exporter/file_append_mode branch from 4d6582c to a7b43a5 Compare March 1, 2024 12:37
@OverOrion
Copy link
Contributor Author 

Error: fileexporter/factory.go:132:33: param append has same name as predeclared identifier (predeclared)

So I changed append to shouldAppend, hope that is okay.

@djaglowski djaglowski merged commit 75cfca8 into open-telemetry:main Mar 6, 2024
142 checks passed
@github-actions github-actions bot added this to the next release milestone Mar 6, 2024
DougManton pushed a commit to DougManton/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
@OverOrion @DougManton
[exporter/file] add append mode (open-telemetry#31369)
2a2e45f 
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
This adds a new option for configuring the append / truncate behavior of
the fileexporter.

**Link to tracking Issue:**
open-telemetry#31364

**Testing:** Added `TestAppend` unit test and manually tested using
`telemetrygen` and the following configuration:

```yaml
receivers:
  otlp:
    protocols:
      grpc:
        endpoint: 0.0.0.0:4317

exporters:
  file:
    path: ./receiver_output_append.log
    append: true
service:
  telemetry:
    metrics:
      level: detailed
      address: 0.0.0.0:9998
  pipelines:
    logs:
      receivers: [otlp]
      exporters: [file]
```

**Documentation:** <Describe the documentation added.>

TODO: 

- [x] add documentation once we reached agreement regarding
implementation / naming

Signed-off-by: Szilard Parrag <[email protected]>
XinRanZhAWS pushed a commit to XinRanZhAWS/opentelemetry-collector-contrib that referenced this pull request Mar 13, 2024
@OverOrion @XinRanZhAWS
[exporter/file] add append mode (open-telemetry#31369)
dd949ab 
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
This adds a new option for configuring the append / truncate behavior of
the fileexporter.

**Link to tracking Issue:**
open-telemetry#31364

**Testing:** Added `TestAppend` unit test and manually tested using
`telemetrygen` and the following configuration:

```yaml
receivers:
  otlp:
    protocols:
      grpc:
        endpoint: 0.0.0.0:4317

exporters:
  file:
    path: ./receiver_output_append.log
    append: true
service:
  telemetry:
    metrics:
      level: detailed
      address: 0.0.0.0:9998
  pipelines:
    logs:
      receivers: [otlp]
      exporters: [file]
```

**Documentation:** <Describe the documentation added.>

TODO: 

- [x] add documentation once we reached agreement regarding
implementation / naming

Signed-off-by: Szilard Parrag <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrzej-stencel andrzej-stencel andrzej-stencel approved these changes

@djaglowski djaglowski djaglowski approved these changes

@codeboten codeboten Awaiting requested review from codeboten

@atingchen atingchen Awaiting requested review from atingchen

Assignees

@Aneurysm9 Aneurysm9

Labels
exporter/file
Projects
None yet
Milestone
v0.97.0
Development

Successfully merging this pull request may close these issues.
5 participants
@OverOrion @andrzej-stencel @djaglowski @atingchen @Aneurysm9