Add context to Sentry Hook

[mpass99]

Closes #326

With updating our Sentry Hook, tracing information stored in the passed context can be associated with sentry events/issues.

With this first commit, we added the context just to log statements where an context was available. In many places we do not pass the (request) context. I think it would be a good practice to do so in the future, (not only) because we then could associate Sentry issues with Sentry Traces. We may use this PR for a refactoring into the direction of more context passing.

I noticed, that Sentry developed its own Logrus Hook 4 months ago. We may think about switching to that. At this moment their implementation does not support the tracing association but there is an open PR getsentry/sentry-go#522

[MrSerth]

Nice, thanks for continuing your efforts to get Sentry right.

I am still wondering about the need to pass the request context just everywhere (shouldn't that just be available "globally"?), but I am fine doing so. One idea to reduce the need to pass the context as a parameter could be to attach it as an attribute to the objects that we use...

I was just checking the list of log occurrences and thought some additional might get a context. Precisely, I thought of the following:

• internal/api/api.go:45
• internal/api/websocket.go:50
• internal/api/websocket.go:65 # might be more difficult
• internal/api/websocket.go:71 # might be more difficult
• internal/api/websocket.go:100
• internal/api/ws/codeocean_reader.go:100
• internal/api/ws/codeocean_reader.go:113
• internal/api/ws/codeocean_writer.go:130
• internal/api/ws/codeocean_writer.go:142
• internal/api/ws/codeocean_writer.go:146
• internal/api/ws/codeocean_writer.go:150
• internal/nomad/api_querier.go:94
• internal/nomad/api_querier.go:132
• internal/nomad/api_querier.go:135
• internal/runner/nomad_runner.go:291
• internal/runner/nomad_runner.go:293

Sure, not every occurrence of log is a good indicator, as some tasks happen asynchronously outside a request (such as then handling of Nomad events when a new runner was started). I tried filtering the list above; and some might not be achievable easily (which you might want to skip), but others might work quite easily.

Finally, I was wondering whether we need to extract the transaction ID ourselves. In the Logrus integration you mentioned, their library is doing so (for example, see here). I might work already the current way; I haven't tested it. Once the PR you referenced is merged, we might also want to evaluate switching to their Logrus integration, as this will potentially be maintained by them in the future.

[codecov]

Codecov Report

Merging #336 (535ee22) into main (830b361) will increase coverage by 0.06%.
The diff coverage is 65.73%.

@@            Coverage Diff             @@
##             main     #336      +/-   ##
==========================================
+ Coverage   74.31%   74.37%   +0.06%     
==========================================
  Files          37       37              
  Lines        3290     3306      +16     
==========================================
+ Hits         2445     2459      +14     
- Misses        680      682       +2     
  Partials      165      165              

Impacted Files	Coverage Δ
internal/nomad/nomad.go	76.81% <0.00%> (ø)
internal/nomad/sentry_debug_writer.go	79.71% <0.00%> (ø)
pkg/monitoring/influxdb2_middleware.go	57.84% <0.00%> (ø)
pkg/nullio/ls2json.go	70.09% <0.00%> (ø)
internal/api/api.go	73.58% <20.00%> (ø)
internal/api/ws/codeocean_reader.go	64.44% <28.57%> (ø)
internal/runner/nomad_runner.go	79.52% <33.33%> (ø)
internal/api/environments.go	70.40% <50.00%> (ø)
internal/runner/aws_runner.go	58.33% <50.00%> (ø)
internal/nomad/api_querier.go	55.62% <66.66%> (ø)
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[codeclimate]

Code Climate has analyzed commit 535ee22 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 68.5% (65% is the threshold).

This pull request will bring the total coverage in the repository to 71.1% (0.0% change).

View more on Code Climate.

[MrSerth]

Awesome, thank you! I am really looking forward to having all those nice enhancements in Sentry. (I am still somewhat surprised by the need to pass the context to each location manually, but have accepted that for now.)

When checking Sentry, we still have the issue of two errors being shown (as expected), but at least they now seem to be connected 💪 :

Example