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

jicofo, jigasi, jvb: fix SENTRY_DSN not being read #1656

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

DanielMcAssey
Copy link
Contributor

This should fix #1655

@saghul saghul merged commit 8415c84 into jitsi:master Dec 1, 2023
1 check passed
@saghul
Copy link
Member

saghul commented Dec 1, 2023

Thank you!

@krombel
Copy link
Contributor

krombel commented Dec 1, 2023

There are several flaws in this PR apparently...

  1. It is now not possible anymore to have dedicated SENTRY_DSNs for the different containers
  2. Why all services now default to JICOFO_SENTRY_DSN - although the service is jigasi or jvb...

@saghul
Copy link
Member

saghul commented Dec 1, 2023

You are right, I pulled the trigger too quick 😅

Indeed we should keep thedifferent DSNs a possibility.

@DanielMcAssey
Copy link
Contributor Author

There are several flaws in this PR apparently...

  1. It is now not possible anymore to have dedicated SENTRY_DSNs for the different containers
  2. Why all services now default to JICOFO_SENTRY_DSN - although the service is jigasi or jvb...

For 1, its hasn't been possible to have separate SENTRY_DSN with the previous configuration. This just solves the issue of actually enabling SENTRY_DSN

For 2, the reason it defaults to JICOFO_SENTRY_DSN is for backwards compatibility as referenced in #342 (comment)

@saghul
Copy link
Member

saghul commented Dec 1, 2023

For 1, its hasn't been possible to have separate SENTRY_DSN with the previous configuration. This just solves the issue of actually enabling SENTRY_DSN

The intent was there. If SENTRY_DSN wasn't set, it was set to a default based on the service, else to 0.

@DanielMcAssey
Copy link
Contributor Author

DanielMcAssey commented Dec 1, 2023

So I can update the PR to make individual SENTRY_DSNs, but it would break backwards compat with JICOFO_SENTRY_DSN as it would involve remapping env variables in docker-compose.yaml

Ultimately the Sentry library is looking for the SENTRY_DSN environment variable, so either we export it manually during a run command, or remap in docker-compose, I don't mind doing either.

Ignore me, opened a new PR, kept my previous comment for posterity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SENTRY_DSN can not be defined
3 participants