jicofo icon indicating copy to clipboard operation
jicofo copied to clipboard

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

Open bgrozev opened this issue 3 years ago • 7 comments

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

bgrozev avatar Dec 17 '21 20:12 bgrozev

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%) :arrow_down:

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.

codecov[bot] avatar Dec 17 '21 20:12 codecov[bot]

@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.

sapkra avatar Dec 18 '21 06:12 sapkra

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

damencho avatar Dec 18 '21 13:12 damencho

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

bgrozev avatar Dec 19 '21 18:12 bgrozev

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.

saghul avatar Dec 20 '21 10:12 saghul

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.

bgrozev avatar Dec 20 '21 18:12 bgrozev

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.

saghul avatar Dec 20 '21 19:12 saghul