Skip to content

Conversation

@dcramer
Copy link
Member

@dcramer dcramer commented Mar 10, 2022

The motivation around this is driven by the principal of keeping development an MVP and lightweight. While Relay itself may not be a large resource consumer, the more exceptions we make, the more they stack up. Even when things aren't utilizing huge amounts of resources, they are still creating complexity in workflows. For example, I was developing an entirely UI-driven feature (debugging TS compile errors), and Relay kept filling up my terminal with its logs.

See also getsentry/getsentry#7136

@markstory
Copy link
Member

What do you see as 'typical development'? Product features like alerts, performance and issues often need to ingest events locally in order to trigger alerts, or create platform specific events (eg. native).

Without also adding a --with-relay flag to sentry devserver or enabling relay when --workers is enabled folks will need to modify both config and use CLI flags to do end-to-end testing locally. This could be more challenging for new folks as I'm pretty sure onboarding has them ingesting events locally.

@dcramer
Copy link
Member Author

dcramer commented Mar 10, 2022

@markstory those should be TDD IMO. I wanted to add --with, but theres some complexity and I didn't want to turn this into a large refactor.

Generally speaking I see dev as three things:

  1. E2E QA, which we do very little of and is often not super needed
  2. UI development which should be entirely mock driven
  3. Systems which should have full test driven outcomes, and be QA'd as needed

Ideally 2/3 are checked against each other (outputs/inputs), but if you can keep API contracts light it solves for it.

The main thing is we cannot run production in development as a principal and I see this as a key step in enforcing that guideline.

@dcramer
Copy link
Member Author

dcramer commented Mar 10, 2022

also fwiw I'm ok if we say we want a --with-relay flag as the better solution to SENTRY_USE_RELAY (but not removing the setting at this time), and I can package that up in here easy enough.

@dcramer
Copy link
Member Author

dcramer commented Mar 10, 2022

per jan we should consider controlling kafka as well (this might be harder), but at the very least making this more comprehensive

I will update with:

  1. a relay/ingest flag on devserver
  2. ideally a way to disable relay-only services when relay is disabled (via devserver only)

@jan-auer
Copy link
Member

jan-auer commented Mar 10, 2022

SENTRY_USE_RELAY already controls Kafka, ingest workers (celery tasks) and the Relay devservices container. Would suggest to update the doc comment on the setting, and if you add sth like --with-ingest make sure it triggers exactly what the flag does (ideally just flip the flag early enough in bootstrapping).

@jan-auer
Copy link
Member

Follow-up thought: Folks who need to run with ingest enabled likely need this permanently or at least for longer periods of time. I think you're good to merge as-is, as long as the comment + develop docs (getsentry/develop#520) are clear enough.

@dcramer dcramer force-pushed the feat/disable-relay-default branch from 4e5b7fa to 94357f6 Compare March 10, 2022 20:31
@dcramer
Copy link
Member Author

dcramer commented Mar 10, 2022

I've added the flag as well to make this more flexible for dev workflows. IMO always have a setting for your local env + a flag is ideal.

The motivation around this is driven by the principal of keeping development an MVP and lightweight. While Relay itself may not be a large resource consumer, the more exceptions we make, the more they stack up. Even when things aren't utilizing huge amounts of resources, they are still creating complexity in workflows. For example, I was developing an entirely UI-driven feature (debugging TS compile errors), and Relay kept filling up my terminal with its logs.

- `SENTRY_USE_RELAY` defaults to `False`.
- Add `--relay` and `--no-relay` for overriding setting.
Copy link
Member

@chadwhitacre chadwhitacre left a comment

Choose a reason for hiding this comment

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

This is a breaking change for self-hosted, yes? I/we can add SENTRY_USE_RELAY=True to the example conf over there, but the key is not set at all currently in the example which means that existing deployments will have the setting changed for them with this PR unless they take action to update their local sentry.conf.py. Am I reading this right?

@chadwhitacre
Copy link
Member

unless they take action

Alternately we can try to be creative and modify their sentry.conf.py for them or at least grep RUN_RELAY sentry/sentry.conf.py || echo "Please update your config".

@dcramer
Copy link
Member Author

dcramer commented Mar 10, 2022

I dont see any references to prod code for this behavior:

image

Copy link
Member

@chadwhitacre chadwhitacre left a comment

Choose a reason for hiding this comment

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

False alarm. Devserver is the only thing that consumes SENTRY_USE_RELAY. 👍

@dcramer dcramer merged commit ba01ef9 into master Mar 10, 2022
@dcramer dcramer deleted the feat/disable-relay-default branch March 10, 2022 21:24
@github-actions github-actions bot locked and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants