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

feat(nodestore): A file system-based node storage backend, with added support for S3 and GCS storage #76250

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat(nodestore): file system-based nodestore backend, with added supp…
…ort for S3 and GCS storage.
  • Loading branch information
klboke committed Aug 15, 2024
commit e22aea8e56dd0be9f7ef763d3ef05f01f55a6a55
91 changes: 48 additions & 43 deletions src/sentry/nodestore/filesystem/backend.py
Original file line number Diff line number Diff line change
@@ -1,53 +1,58 @@
import os
from datetime import datetime, timedelta, timezone
import datetime
from io import BytesIO

from django.conf import settings

from sentry import options as options_store
from sentry.models.file import get_storage
from sentry.nodestore.base import NodeStorage


class FileSystemNodeStorage(NodeStorage):
"""
A simple backend that saves each node as a file. Only appropriate for
debugging and development!
A file system-based backend that saves each node as a file, Local fileSystem only appropriate for
debugging and development! For production environments, please use S3 or GCS. config.yml Configuration reference:
>>> filestore.backend: "s3"
... filestore.options:
... access_key: "xxx"
... secret_key: "xxx"
... endpoint_url: "https://s3.us-east-1.amazonaws.com"
... bucket_name: "sentry"
... location: "/sentry"
"""

def __init__(self, path: str | None = None):
self.path: str = ""

if not settings.DEBUG:
raise ValueError("FileSystemNodeStorage should only be used in development!")
if path:
self.path = os.path.abspath(os.path.expanduser(path))
else:
self.path = os.path.abspath(os.path.join(os.path.dirname(__file__), "./nodes"))

def _get_bytes(self, id: str) -> bytes:
with open(self.node_path(id), "rb") as file:
return file.read()

def _set_bytes(self, id: str, data: bytes, ttl: timedelta | None = None) -> None:
with open(self.node_path(id), "wb") as file:
file.write(data)

def delete(self, id: str) -> None:
os.remove(self.node_path(id))

def cleanup(self, cutoff: datetime) -> None:
for filename in os.listdir(self.path):
path = os.path.join(self.path, filename)
creation_datetime = datetime.fromtimestamp(os.path.getctime(path)).replace(
tzinfo=timezone.utc
)

if creation_datetime > cutoff:
os.remove(path)

def bootstrap(self) -> None:
try:
os.mkdir(self.path)
except FileExistsError:
pass

def node_path(self, id: str) -> str:
return os.path.join(self.path, f"{id}.json")
def __init__(self, prefix_path=None):
self.prefix_path: str = "nodestore/"
if not settings.DEBUG and options_store.get("filestore.backend") == "filesystem":
raise ValueError("Local fileSystem should only be used in development!")
if prefix_path:
Comment on lines +26 to +27
Copy link
Contributor

Choose a reason for hiding this comment

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

This would break existing self-hosted users that don't use (or don't have access to) S3 compatible services. This should just log a warning instead of throwing a ValueError.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. The default NodeStore is sentry.nodestore.django.DjangoNodeStorage rather than this. So it won't break any existing users. If you want to configure this FileSystemNodeStorage in non-debug environment, you must configure FileStore also.

Copy link

Choose a reason for hiding this comment

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

Ping to bump this PR.

I would really like to see the built-in support for S3 Node Store (even though performance is not endorsed).

I think the concern here is: FileSystemNodeStorage by title is supposed to be using "File System". It is not intuitive that by enabling "S3 File Store", FileSystemNodeStore will push things to S3.

I think better way is to explicitly create a new S3NodeStore class with those logic.

Copy link

@rophy rophy Dec 15, 2024

Choose a reason for hiding this comment

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

See if #82126 makes sense to everyone.

self.prefix_path = prefix_path

def _get_bytes(self, id: str):
storage = get_storage()
path = self.node_path(id)
return storage.open(path).read()

def _set_bytes(self, id: str, data: bytes, ttl=0):
storage = get_storage()
path = self.node_path(id)
storage.save(path, BytesIO(data))

def delete(self, id):
storage = get_storage()
path = self.node_path(id)
storage.delete(path)

def cleanup(self, cutoff: datetime.datetime):
"""
This driver does not have managed TTLs. To enable TTLs you will need to enable it on your
bucket.
"""
raise NotImplementedError

def bootstrap(self):
# Nothing for filesystem backend to do during bootstrap
pass

def node_path(self, id: str):
return f"{self.prefix_path}{id}.json"
Loading