@szokeasaurusrex Is it only errors passing through issues_sampler or also other event types?

If it's only errors, calling the option error_sampler might be better for clarity.

More importantly, are the events from the original issue errors (as opposed to e.g. messages) and would they pass through the issues_sampler?

An issue in Sentry represents N events, so calling it issue_sampler isn't the best way forward. IMO, this option should sample everything that ends up in capture_event.

@szokeasaurusrex Is it only errors passing through issues_sampler or also other event types?

If it's only errors, calling the option error_sampler might be better for clarity.

The _should_sample_error function that I have modified here is called in the capture_event functions, so I believe all events will be sampled. In light of @cleptric's comment, though, we might consider renaming issues_sampler to something like sampler. What do you think?

More importantly, are the events from the original issue errors (as opposed to e.g. messages) and would they pass through the issues_sampler?

I am unsure given the information I see in the original issue. @saippuakauppias, could you please provide the code where these errors/messages originate from, or provide us more detail with how these are being generated, so we can answer this question?

The _should_sample_error function that I have modified here is called in the capture_event functions, so I believe all events will be sampled. In light of @cleptric's comment, though, we might consider renaming issues_sampler to something like events_sampler. What do you think?

Yes, looks like it's everything but transactions and checkins:

In that case events_sampler/event_sampler sounds good! And it also means that it can be used to set the sample rate for any event, so that answers my second question.

IMO, this option should sample everything that ends up in capture_event.

@cleptric Checkins and transactions also end up in capture_event but wouldn't go through the sampler, which is ok I think. For transactions there is traces_sampler/traces_sample_rate and I assume there's currently no demand for being able to set checkin sample rates. And if there is, before_send is still a thing.

Once we've settled on the API please don't forget to update the docs @szokeasaurusrex 📖

I discussed with @cleptric, and we now think we should pick a different name than event_sampler, since transactions are also technically events.

The best idea we came up with was to just call the new sampler sampler. This name would also fit the pattern where our sampler corresponds to the sample_rate the same way the traces_sampler corresponds to the traces_sample_rate. Do you have any other ideas @sentrivana, or are you okay with us proceeding naming the sampler sampler?

I am unsure given the information I see in the original issue. @saippuakauppias, could you please provide the code where these errors/messages originate from, or provide us more detail with how these are being generated, so we can answer this question?

I'm using django, so it's going to be a bit of a specific example here, but I think it's reproducible.

# settings/production.py

LOGGING = {
  'version': 1,
  'formatters': {
    'simple': {
        'format': '[%(asctime)s] %(levelname)s %(module)s %(message)s',
        'datefmt': '%Y-%m-%d %H:%M:%S'
    },
  },

  'handlers': {
    'sentry': {
      'level': 'WARNING',
      'class': 'base.sentry.SentryHandler',
    },
    'streamlogger': {
      'level': 'DEBUG',
      'class': 'logging.StreamHandler',
      'formatter': 'simple'
    },
  },

  'loggers': {
    'warn.logger': {
      'handlers': ['sentry', 'streamlogger'],
      'level': 'WARNING',
      'propagate': False
    },
  },
  
  'root': {
    'level': 'INFO',
    'handlers': ['streamlogger'],
  },
}

# base/sentry.py
from sentry_sdk.integrations.atexit import AtexitIntegration
from sentry_sdk.integrations.dedupe import DedupeIntegration
from sentry_sdk.integrations.django import DjangoIntegration
from sentry_sdk.integrations.excepthook import ExcepthookIntegration
from sentry_sdk.integrations.logging import EventHandler, LoggingIntegration
from sentry_sdk.integrations.modules import ModulesIntegration
from sentry_sdk.integrations.stdlib import StdlibIntegration


class SentryHandler(EventHandler):
    ...


def initialize(dsn, service, release, branch, debug: bool, breadcrumb_filters: list[dict] = None):
    from django.conf import settings

    sample_events_by_path = {
        f'{settings.PROJECT_ROOT}/some/module.py': 0.0001,
    }

    def prepare_event(event, hint):
        request_data = get_request_data()
        tags = event.setdefault('tags', {})

        # my hack for filtration here
        is_sampled = False
        if isinstance(hint, dict) and 'log_record' in hint:
            pathname = getattr(hint['log_record'], 'pathname', '')
            is_sampled = pathname in sample_events_by_path
            sample_rate = sample_events_by_path.get(pathname, 1)

            if sample_rate < 1.0 and random.random() < sample_rate:
                return None

        tags['is_sampled'] = is_sampled
        
        return event

    sentry_sdk.init(
        dsn=dsn,
        release=release or None,
        before_send=prepare_event,
        default_integrations=False,
        integrations=[
            StdlibIntegration(),
            ExcepthookIntegration(),
            DedupeIntegration(),
            AtexitIntegration(),
            ModulesIntegration(),
            DjangoIntegration(),
            LoggingIntegration(level=None, event_level=None),
        ],
    )

# some/module.py
import logging
import requests


logger = logging.getLogger('warn.logger')


def some_fun():
    try:
        resp = requsts.get('http://some.service.name')
        resp.raise_for_status()
    except Exception as e:
        logger.warning('Error get value from service', exc_info=True)

I discussed with @cleptric, and we now think we should pick a different name than event_sampler, since transactions are also technically events.

The best idea we came up with was to just call the new sampler sampler. This name would also fit the pattern where our sampler corresponds to the sample_rate the same way the traces_sampler corresponds to the traces_sample_rate. Do you have any other ideas @sentrivana, or are you okay with us proceeding naming the sampler sampler?

I'm not opposed to calling it sampler per se, but I don't follow the reasoning. If the issue with event_sampler is that it's too broad, then sampler sounds even broader? 😄

FWIW I don't think there's an ideal solution here. But if I saw sampler as a user without reading the docs, I would assume it samples everything and is a generalization of/supersedes traces_sampler. On the other hand, event_sampler suggests some sort of dichotomy to traces_sampler -- i.e., those two probably sample different things.

I discussed with @antonpirker this morning about naming the sampler as sampler versus events_sampler, he also was leaning towards events_sampler. Since the majority consensus here seems to be to go with events_sampler, let's go with events_sampler for now.

Thank you so much for implementing this idea!

Can you please tell me if I can hope that soon you will release a new version of the package so that I can install it from PyPI?

@saippuakauppias We typically release new versions of the Python SDK every few weeks, so hopefully you will be able to install it from PyPI soon