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

Sentry.io support to monitor errors #7667

Closed
wants to merge 7 commits into from
Closed

Sentry.io support to monitor errors #7667

wants to merge 7 commits into from

Conversation

21h
Copy link
Contributor

@21h 21h commented Jul 29, 2019

At startup check SENTRY_DSN env variable to activate sentry errors logging. Useful for investigation of bugs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 29, 2019
21h and others added 2 commits July 30, 2019 01:24
Co-Authored-By: Lauris BH <lauris@nix.lv>
@codecov-io
Copy link

codecov-io commented Jul 29, 2019

Codecov Report

Merging #7667 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7667      +/-   ##
==========================================
+ Coverage   41.41%   41.41%   +<.01%     
==========================================
  Files         474      474              
  Lines       63876    63876              
==========================================
+ Hits        26453    26455       +2     
+ Misses      33978    33975       -3     
- Partials     3445     3446       +1
Impacted Files Coverage Δ
models/unit.go 67.56% <0%> (+5.4%) ⬆️

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 cde95f9...d8772f2. Read the comment docs.

@lafriks lafriks added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jul 29, 2019
@lafriks lafriks added this to the 1.10.0 milestone Jul 29, 2019
@zeripath
Copy link
Contributor

zeripath commented Jul 31, 2019

Sentry requires that you actually tell it about errors - it's not magic.

So you would have to next either add multiple specific sentry reporting blocks everywhere that an error is found or, better, just create a sentry logger provider to get it to report an errors.

To then make it really useful requires adding context - this is where the real problems start - you're going to have to place sentry calls almost everywhere to give it context. This might be possible with a macaron.Handler at an appropriate level I guess(?) but it might require changing the macaron router code to actually properly handle breadcrumbs, and the login process etc.

But all of this, placing code at key points in our system, is to provide logging to a proprietary (Edit:) another logging system.

Now... I agree we can do logging better. I agree with Sentry's idea about context - and have been thinking about it quietly for some time (albeit from a python twisted view). However I do not believe that tightly integrating a proprietary system like this in to an open source project like Gitea is necessarily appropriate. (Edit) Tightly integrating another logging system might not necessarily be the correct thing to do - although I'm not fundamentally against this I think we need to be careful about what we integrate.

Now if we can do this in such a way to improve logging in general - then great let's go for it.


My initial reading of the Sentry documentation made me think that you could not host a DSN yourself and that you had to pass data up to their services. I was incorrect.

@lafriks
Copy link
Member

lafriks commented Jul 31, 2019

@zeripath I have not used it but imho sentry is open source

Offtopic: I for one would like to see Elastic APM integrated

@zeripath
Copy link
Contributor

@lafriks - ah sorry I missed that you could host a DSN yourself. A quick scan through the documentation implied that you had to pass stuff to their DSN.

// Sentry.io logging if user wants to investigate internal errors
sentryDSN := os.Getenv("SENTRY_DSN")
if sentryDSN != "" {
if err := sentry.Init(sentry.ClientOptions{
Copy link
Member

Choose a reason for hiding this comment

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

We still need repanic: true ?

@21h
Copy link
Contributor Author

21h commented Jul 31, 2019

sentry is a nice choice to get events, it really helps to find problems with code or server itself.

@lunny lunny modified the milestones: 1.10.0, 1.11.0 Sep 15, 2019
@stale
Copy link

stale bot commented Nov 14, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Nov 14, 2019
@techknowlogick techknowlogick modified the milestones: 1.11.0, 1.x.x Dec 12, 2019
@stale stale bot removed the issue/stale label Dec 12, 2019
@stale
Copy link

stale bot commented Feb 10, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Feb 10, 2020
@lafriks lafriks removed this from the 1.x.x milestone Feb 10, 2020
@stale stale bot removed the issue/stale label Feb 10, 2020
@lafriks lafriks closed this Feb 10, 2020
@alexanderadam
Copy link

To then make it really useful requires adding context

I just want to add that, besides error tracking, Sentry is also able to track releases* (I would also love to have a gitea webhook for that) and deploys.
So I guess it would be nice as well to see when a certain gitea version was deployed if there is suddenly unwanted behaviour.

And yes, as you correctly pointed out, Sentry is open source and you can simply host it on your own with the official Docker images for example. Similar to gitea.

*I'm not quite sure whether sentry-go is able to track releases?

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants