open-telemetry / opentelemetry-collector-contrib

Contrib repository for the OpenTelemetry Collector
https://opentelemetry.io
Apache License 2.0
3.13k stars 2.4k forks source link

[receiver/filelog] Open files on Windows with FILE_SHARE_DELETE #32037

Open BinaryFissionGames opened 8 months ago

BinaryFissionGames commented 8 months ago

Component(s)

receiver/filelog

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

On Windows, files open by the collector cannot be moved/deleted. It would be nice if the collector opened a file with the FILE_SHARE_DELETE share mode

Describe the solution you'd like

When opening files on Windows, with the CreateFile(W|A) function (docs here), a share mode is passed in that dictates how other processes may access this file.

The documentation calls out 3 values that may be combined, "FILE_SHARE_DELETE", "FILE_SHARE_READ", and "FILE_SHARE_WRITE". In Go's default os.Open implementation, "FILE_SHARE_READ|FILE_SHARE_WRITE" is used as this parameter, but having "FILE_SHARE_DELETE" would be useful, because it would allow other processes to delete or rename the file without having to shut down the collector.

It should be possible to construct a file handle using the CreateFile syscall, and turn it into an os.File by using syscall.CreateFile to create the handle, then wrap it using windows.NewFile (the fd here is not of type syscall.Handle, but the implementation is clear that fd should be a handle)

Describe alternatives you've considered

No response

Additional context

No response

github-actions[bot] commented 8 months ago

Pinging code owners:

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

djaglowski commented 8 months ago

I really like the idea of doing this, assuming it works. It would allow better support on windows and likely would simplify a few aspects of our codebase as well, for example where we currently manage files differently on windows.

cc: @pjanotti, @ChrsMark, @OverOrion

pjanotti commented 8 months ago

This makes sense. We may need to be sure that code reading the file is hardened for read failures, but, I imagine that is already the case. Anyway, when implementing we should have a test deleting a file while still reading it to validate the error handling.

OverOrion commented 8 months ago

I like the idea as well.

In theory this could allow the Collector to support the delete_after_read setting on Windows as well (IIUC this is currently unsupported on Windows).

github-actions[bot] commented 6 months ago

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

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

github-actions[bot] commented 4 months ago

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

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