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

chore: Remove the sentry and agafua-syslog runtime dependencies. #854

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bgrozev
Copy link
Member

@bgrozev bgrozev commented Dec 17, 2021

This is to stop shipping non-essential jars and reduce potential exposure to vulnerabilities. Setting up sentry requires setting an environment variable (and potentially more changes?), so it should be easy to adapt to also include the sentry jar in the classpath.

cc @saghul @sapkra

@codecov
Copy link

codecov bot commented Dec 17, 2021

Codecov Report

Merging #854 (64c1d24) into master (04c1143) will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #854      +/-   ##
============================================
- Coverage     46.34%   46.25%   -0.09%     
+ Complexity      735      733       -2     
============================================
  Files           106      106              
  Lines          6693     6693              
  Branches        925      925              
============================================
- Hits           3102     3096       -6     
- Misses         3188     3192       +4     
- Partials        403      405       +2     
Impacted Files Coverage Δ
...tsi/jicofo/conference/JitsiMeetConferenceImpl.java 43.62% <0.00%> (-0.93%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04c1143...64c1d24. Read the comment docs.

@sapkra
Copy link
Contributor

sapkra commented Dec 18, 2021

@bgrozev How will I be able to use error tracking in the future? I'm not that familiar with java setups. To enable a sentry handler it needs to be installed somehow.

@damencho
Copy link
Member

If you put the jar file in /usr/share/jicofo/libs before starting it and have the correct logging config, that is enough.

@bgrozev
Copy link
Member Author

bgrozev commented Dec 19, 2021

Or add the sentry jar file to the classpath. We can add an env variable for additional classpath entries if that helps.

@saghul
Copy link
Member

saghul commented Dec 20, 2021

Hasn't Sentry released a fixed version?

We expose this in the Docker setup, so it would be broken now.

Sentry is pretty well regarded in the industry, FWIW.

@bgrozev
Copy link
Member Author

bgrozev commented Dec 20, 2021

How do we expose it in docker? Can we ship the jar and insert it in the classpath in the docker setup (ideally behind a flag, since not every docker uses uses sentry)? I'd be happy to work on this if you can point me to the right place.

I don't doubt that sentry is well regarded. Still, it's additional surface area and an additional dependency to be maintained, which is only used by a very small subset of users. And there's a simple way to avoid it.

@saghul
Copy link
Member

saghul commented Dec 20, 2021

Fair point.

We esport env variables to turn it on, they are part of the config template file.

Yeah, we could ship it there indeed.

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.

4 participants