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 Performance Tracing #275

Merged
merged 5 commits into from
Feb 3, 2023
Merged

Sentry Performance Tracing #275

merged 5 commits into from
Feb 3, 2023

Conversation

mpass99
Copy link
Collaborator

@mpass99 mpass99 commented Feb 2, 2023

Relates to #270

With Sentry's Distributed Tracing we can connect the Tracing of our applications to get a more detailed view into the time behavior.

For Poseidon we just have to enable the tracing functionality of Sentry. For that see also: 38ffc3e9.

The tracing connection between the application is done automatically by Sentry (via the trace-id http header; See).

With only this we would still have two separate transactions - one for Submission create and one for Submission Score. To join them we can enable the Sentry Tracing for the JavaScript Frontend.

@mpass99 mpass99 requested a review from MrSerth February 2, 2023 18:27
@mpass99 mpass99 self-assigned this Feb 2, 2023
@MrSerth
Copy link
Member

MrSerth commented Feb 2, 2023

Awesome, thanks for getting that started! I was reviewing commit-by-commit and find all changes plausible. Besides reviewing, I also started having a look at Sentry and was happy to see some traces there ;).

What I was still thinking are two things regarding the spans:

  1. Should we add additional spans, e.g., in some filters running with every request or other often-used methods?
    This would allow us to see more details, such as shown in this trace.
  2. Can we add the current (custom) span names into the "description" field and rather an "identifier" as the current name? This would allow to see the name for spans. For example, compare this custom span to this pre-defined span. We might also come up with an own naming scheme, e.g., poseidon.operation, poseidon.manager, poseidon.storage, poseidon.nomad.execute, influx.send, nomad.query, response.render, ...

Regarding linking: Yes, let's use the trace ID and let's generate it through the JavaScript frontend and pass it along.

@mpass99 mpass99 force-pushed the feature/#270-sentry-performance branch from a7f2ad0 to 554afe3 Compare February 3, 2023 01:35
@mpass99
Copy link
Collaborator Author

mpass99 commented Feb 3, 2023

Good remarks!
I added an identifier for the custom spans.
Also, I added more custom spans for the most used and most time consuming functionality, Execute. If you would like to know the time behavior of other methods, please let me know.

@mpass99 mpass99 force-pushed the feature/#270-sentry-performance branch from 554afe3 to c45bd14 Compare February 3, 2023 01:54
by passing the Sentry Context down our abstraction stack.
This included changes in the complex context management of managing a Command Execution.
@mpass99 mpass99 force-pushed the feature/#270-sentry-performance branch from c45bd14 to 7ab8dca Compare February 3, 2023 02:02
@codeclimate
Copy link

codeclimate bot commented Feb 3, 2023

Code Climate has analyzed commit 7ab8dca and detected 0 issues on this pull request.

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

This pull request will bring the total coverage in the repository to 72.6% (-0.1% change).

View more on Code Climate.

Copy link
Member

@MrSerth MrSerth left a comment

Choose a reason for hiding this comment

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

Awesome! I was just testing the new tracing with CodeOcean, and it works like a charm!

Let's evaluate the responses once we have some data to identify where to add more details. :)

@mpass99 mpass99 merged commit 4550a45 into main Feb 3, 2023
@mpass99 mpass99 deleted the feature/#270-sentry-performance branch February 3, 2023 10:29
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.

2 participants