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.

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

[exporter/file] Add buffer flush interval #18251

Closed
loomis-relativity opened this issue Feb 2, 2023 · 7 comments · Fixed by #20675
Closed

[exporter/file] Add buffer flush interval #18251

loomis-relativity opened this issue Feb 2, 2023 · 7 comments · Fixed by #20675
Labels
enhancement New feature or request exporter/file

Comments

@loomis-relativity
Copy link
Contributor

Component(s)

exporter/file

Is your feature request related to a problem? Please describe.

The observability team at my company maintains a custom version of the collector. The custom collector includes custom receivers, processors, and exporters for all OpenTelemetry data types. The file exporter is used to verify the full data flow by comparing known inputs to their expected output payloads.
In v0.70.0, the file exporter moved to a buffered writer. This breaks the tests because we cannot reliably ensure that the payloads have been written to disk when the check happens. Having the buffered output as the default makes sense; however, we'd also like a mechanism for turning off the buffering for these types of use cases.

Describe the solution you'd like

Add a parameter to turn off the buffering within the file exporter.

Describe alternatives you've considered

We've discussed using the OpenTelemetry testbed or using a real backend for collecting the data. However, both of these solutions would significantly complicate the testing environment.

Additional context

No response

@loomis-relativity loomis-relativity added enhancement New feature or request needs triage New item requiring triage labels Feb 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@djaglowski
Copy link
Member

This breaks the tests because we cannot reliably ensure that the payloads have been written to disk when the check happens.

Can you elaborate on this?

cc: @MovieStoreGuy, I believe #17543 is the relevant change here so maybe you have an opinion too.

@loomis-relativity
Copy link
Contributor Author

@djaglowski To test the full data flow with our custom collector, we deploy it via k8s and compare known payloads to expected outputs. The actual outputs are written to the local disk in a k8s pod via the file exporter. These tests fail with the buffering turned on because the output isn't large enough to fill the buffers, so the actual files on disk remain empty. To be clear, these are our internal, custom tests, not the standard OpenTelemetry collector tests.

@loomis-relativity
Copy link
Contributor Author

When using the file rotation, a writer from lumberjack is used. This implementation writes the payloads to disk with a reasonable delay for testing. For others that might need this, just change the file exporter configuration to:

file:
  path: "./foo"
  rotation:

@djaglowski djaglowski changed the title allow buffering to be turned off in file exporter [exporter/file] Allow buffering to be turned off Feb 9, 2023
@djaglowski
Copy link
Member

I'm reopening this because I think it is a valid issue that should be addressed in some way. I'm glad there is a workaround for now but I'm still hoping @MovieStoreGuy will weigh in on whether this should be a boolean, or timer, or something else.

@djaglowski djaglowski reopened this Feb 9, 2023
@damemi
Copy link
Contributor

damemi commented Mar 24, 2023

Just want to +1 that I hit this issue too. I thought something was wrong with my file exporter config, because I could see output from the logging exporter but nothing written to the file. It would also be helpful if the file exporter had more logging to show that it's processing work.

#18251 (comment) fixed it for me.

As a side note, I wonder why this hasn't hit the e2e tests yet (https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/.github/workflows/e2e-tests.yml), or what is different in their setup that they don't hit this issue. I'm working on a similar test that follows basically the same process based on that.

@djaglowski djaglowski changed the title [exporter/file] Allow buffering to be turned off [exporter/file] Add buffer flush interval Apr 4, 2023
@MovieStoreGuy
Copy link
Contributor

I'm reopening this because I think it is a valid issue that should be addressed in some way. I'm glad there is a workaround for now but I'm still hoping @MovieStoreGuy will weigh in on whether this should be a boolean, or timer, or something else.

Sorry I am late to the party. periodic flushing making sense to me since it is the same as the batch exporter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request exporter/file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants