Skip to content

Conversation

@rads-1996
Copy link
Member

@rads-1996 rads-1996 commented Oct 17, 2025

Description

Adds folder permissions for Unix when multiple users are using the same system.

Fixes - #43414

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@Copilot Copilot AI review requested due to automatic review settings October 17, 2025 21:50
@github-actions github-actions bot added the Monitor - Exporter Monitor OpenTelemetry Exporter label Oct 17, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds multi-user Unix permission handling for local storage by introducing a helper to set parent (0o755) and storage (0o700) directory permissions and adds related test cases.

  • Replaces direct chmod with _set_unix_permissions for Unix paths.
  • Adds three tests covering success and two failure scenarios related to permission setting.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/_storage.py Introduces _set_unix_permissions and updates Unix branch to use it.
sdk/monitor/azure-monitor-opentelemetry-exporter/tests/test_storage.py Adds tests for multi-user Unix permission scenarios and error handling.

@rads-1996 rads-1996 marked this pull request as draft October 17, 2025 22:01
self._storage_directory = kwargs.get("storage_directory")
elif not self._disable_offline_storage:
shared_root = os.path.join(tempfile.gettempdir(), _AZURE_TEMPDIR_PREFIX)
user_segment = getpass.getuser()
Copy link
Member Author

Choose a reason for hiding this comment

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

This might return an exception if the user details are not found, should we skip the user_segment in that case?

Copy link
Member

@hectorhdzg hectorhdzg Oct 20, 2025

Choose a reason for hiding this comment

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

There are other unique properties that can be used, like time, new guid or similar

.NET use a hash with ikey, user name and process name
https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/PersistentStorage/StorageHelper.cs#L25

It will be good to have same logic in all exporters, having some language differentiator as well

Copy link
Member

Choose a reason for hiding this comment

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

Also this issue should be present in Node.js as well, only ikey is used to differentiate folders

Copy link
Member

Choose a reason for hiding this comment

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

Regarding what to do if some information is not available, we should try to build a hash using what is present to ensure problem is less likely to happen

Copy link
Member Author

Choose a reason for hiding this comment

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

Also this issue should be present in Node.js as well, only ikey is used to differentiate folders

Will create a separate task for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are other unique properties that can be used, like time, new guid or similar

.NET use a hash with ikey, user name and process name https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/monitor/Azure.Monitor.OpenTelemetry.Exporter/src/Internals/PersistentStorage/StorageHelper.cs#L25

It will be good to have same logic in all exporters, having some language differentiator as well

When you say language differentiator, do you mean a difference in the functionality across languages or add a language attribute to this directory path?

@rads-1996 rads-1996 marked this pull request as ready for review October 20, 2025 23:09
@berland
Copy link

berland commented Oct 21, 2025

I have tested this PR, but it will still create the directory /tmp/Microsoft and /tmp/Microsoft/AzureMonitor without 0o777 permissions meaning it will fail when other users on the same machine tries to do the same thing.

@rads-1996
Copy link
Member Author

I have tested this PR, but it will still create the directory /tmp/Microsoft and /tmp/Microsoft/AzureMonitor without 0o777 permissions meaning it will fail when other users on the same machine tries to do the same thing.

@berland I’m updating the implementation to align with the .NET SDK approach for creating a unique subdirectory. Once I push these changes, I’ll share an update so you can test and confirm if it works for you.

@berland
Copy link

berland commented Oct 22, 2025

Some comments to the current PR:

  • The subdirectory structure provided by _AZURE_TEMPDIR_PREFIX seems pointless, now that it resides below /tmp/<somelongrandomstring>.
  • Why not use tempfile.TemporaryDirectory() instead to make this PR less complex?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Monitor - Exporter Monitor OpenTelemetry Exporter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants