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

Add a metric to collect the number of background routines #963

Merged
merged 4 commits into from
Aug 19, 2024

Conversation

kokodak
Copy link
Member

@kokodak kokodak commented Aug 13, 2024

What this PR does / why we need it:

This PR adds a metric to collect the number of goroutines attached to a specific Background task.

Prometheus provides its own thread-safe Gauge metric, which can be used to accurately collect the number of concurrent goroutines.

Also, although the only task running in the background so far is PushPull, I implemented the AttachGoroutine() method to take a string called taskType as a parameter, since it seems to be focused on reusability.

In my local Grafana, I observed the metrics with a simple promQL query like the one below.

image

The expected effect is that it will be easy to identify which background tasks are bottlenecking and when. For example, during the logic of saving snapshots, the number of observed goroutines may temporarily increase when the DB bottleneck causes each goroutine to have a longer lifecycle. This kind of analysis would help us prevent such issues.

Which issue(s) this PR fixes:

Fixes #386

Special notes for your reviewer:

If you think there's a better metric or promQL to get more meaningful data, please feel free to comment.

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced background goroutine management with improved metrics tracking.
    • Added functionality to monitor and categorize background tasks through Prometheus.
  • Improvements

    • Updated function signatures to incorporate metrics tracking for AttachGoroutine, PushPull, and other related functions.
    • Introduced better observability for background processing tasks, aiding in performance tracking.

These changes significantly enhance the ability to manage and monitor background tasks within the application.

@kokodak kokodak self-assigned this Aug 13, 2024
Copy link

coderabbitai bot commented Aug 13, 2024

Walkthrough

The recent changes enhance the tracking of background goroutines by modifying key methods to accept metrics and task type parameters. This allows for improved monitoring of background tasks via Prometheus, facilitating better management and observability of worker counts. Such updates ensure that metrics accurately reflect the state of goroutines, thus supporting proactive performance management.

Changes

Files Change Summary
server/backend/background/background.go, server/packs/packs.go Enhanced methods to include metrics tracking and identifiers, allowing for improved observability of background tasks.
server/backend/backend.go Updated background instance creation to include metrics, improving task management capabilities.
test/integration/retention_test.go, test/integration/snapshot_test.go Updated tests to reflect new method signatures for better metrics integration.

Assessment against linked issues

Objective Addressed Explanation
Collect Worker Count Metric that Runs Through the Background Package (#386)

Poem

🐇 In the garden where shadows play,
Metrics dance and count the day.
With each task that hops along,
Background workers sing their song.
Watch them flourish, one and all,
Thanks to changes, stand up tall! 🌼✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Outside diff range, codebase verification and nitpick comments (4)
server/backend/background/background.go (2)

28-28: Consider Import Alias for Prometheus Package.

To improve code readability and avoid potential conflicts, consider using an alias for the prometheus package, especially if it is used frequently throughout the codebase.

import prom "github.com/yorkie-team/yorkie/server/profiling/prometheus"

64-68: Update Calls to AttachGoroutine with New Parameters

The AttachGoroutine method has been updated to include metrics and taskType as parameters. However, the call to this method in server/packs/packs.go does not pass these new parameters. Please update this call to include the necessary arguments.

  • File: server/packs/packs.go
    • Line: AttachGoroutine call
Analysis chain

Ensure Proper Usage of New Parameters in AttachGoroutine.

The AttachGoroutine method now accepts metrics and taskType. Verify that all calls to this method throughout the codebase are updated to pass these parameters correctly.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all calls to `AttachGoroutine` are updated with new parameters.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type go -A 3 $'AttachGoroutine('

Length of output: 121


Script:

#!/bin/bash
# Description: Verify all calls to `AttachGoroutine` are updated with new parameters.

# Test: Search for the function usage with corrected regex pattern. Expect: Only occurrences of the new signature.
rg --type go -A 3 'AttachGoroutine\('

Length of output: 567

server/profiling/prometheus/metrics.go (2)

39-39: Add Documentation for taskTypeLabel.

Consider adding a comment to document the purpose of taskTypeLabel for future maintainability.

// taskTypeLabel is used to categorize metrics by task type.

65-66: Ensure Consistency in Metrics Naming.

The naming of backgroundGoroutinesTotal should be consistent with other metrics. Consider adding a prefix or suffix if needed.

// Consider renaming for consistency, e.g., "background_goroutines_total"
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 80c6ea0 and 09aa951.

Files selected for processing (3)
  • server/backend/background/background.go (3 hunks)
  • server/packs/packs.go (1 hunks)
  • server/profiling/prometheus/metrics.go (4 hunks)
Additional comments not posted (3)
server/backend/background/background.go (1)

83-86: Ensure Metrics are Updated on Goroutine Exit.

The deferred function ensures that metrics are decremented when a goroutine exits. This is a good practice to maintain accurate metrics.

server/packs/packs.go (1)

207-207: Verify Metrics and Task Type Parameters in AttachGoroutine Call.

Ensure that the metrics and "pushpull" parameters passed to AttachGoroutine are correct and consistent with the intended monitoring logic.

Verification successful

Parameters in AttachGoroutine Call Verified

The parameters be.Metrics and "pushpull" are correctly used in the AttachGoroutine call, consistent with the intended monitoring logic. No issues were found.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of metrics and task type parameters in `AttachGoroutine` calls.

# Test: Search for the specific usage in the file. Expect: Correct parameters.
rg --type go -A 3 $'AttachGoroutine(func(ctx context.Context) {'

Length of output: 215


Script:

#!/bin/bash
# Description: Verify the correctness of metrics and task type parameters in `AttachGoroutine` calls.

# Test: Search for the specific usage in the file. Expect: Correct parameters.
rg --type go -A 5 'AttachGoroutine\(func\(ctx context.Context\) \{' server/packs/packs.go

Length of output: 268


Script:

#!/bin/bash
# Description: Verify the correctness of metrics and task type parameters in `AttachGoroutine` calls.

# Test: Search for the specific usage in the file with extended context lines. Expect: Correct parameters.
rg --type go -A 10 'AttachGoroutine\(func\(ctx context.Context\) \{' server/packs/packs.go

Length of output: 342


Script:

#!/bin/bash
# Description: Verify the correctness of metrics and task type parameters in `AttachGoroutine` calls.

# Test: Search for the specific usage in the file with more context lines. Expect: Correct parameters.
rg --type go -A 15 -B 5 'AttachGoroutine\(func\(ctx context.Context\) \{' server/packs/packs.go

Length of output: 636


Script:

#!/bin/bash
# Description: Search for the specific parameters `be.Metrics` and `"pushpull"` in the file to locate their usage in `AttachGoroutine`.

rg --type go 'be.Metrics' server/packs/packs.go
rg --type go '"pushpull"' server/packs/packs.go

Length of output: 595

server/profiling/prometheus/metrics.go (1)

140-145: Review Initialization of backgroundGoroutinesTotal.

Ensure that the initialization of backgroundGoroutinesTotal is correct and aligns with the intended metric collection strategy.

Verification successful

Initialization of backgroundGoroutinesTotal is correct.

The backgroundGoroutinesTotal is properly initialized using promauto.With(reg).NewGaugeVec with the necessary options and labels, aligning with the intended metric collection strategy. No issues found.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the initialization of `backgroundGoroutinesTotal`.

# Test: Check for correct initialization. Expect: Proper namespace and labels.
rg --type go -A 5 $'backgroundGoroutinesTotal: promauto.With(reg).NewGaugeVec'

Length of output: 79


Script:

#!/bin/bash
# Description: Search for the initialization and usage of `backgroundGoroutinesTotal` in the specified file.

# Locate the file `server/profiling/prometheus/metrics.go`
fd 'metrics.go' --exec rg 'backgroundGoroutinesTotal' {}

Length of output: 292

server/backend/background/background.go Outdated Show resolved Hide resolved
server/profiling/prometheus/metrics.go Show resolved Hide resolved
server/profiling/prometheus/metrics.go Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
test/integration/retention_test.go (1)

48-53: Prometheus Metrics Integration Issue in AttachGoroutine

The AttachGoroutine method in server/packs/packs.go does not currently pass Prometheus metrics, which are expected as per the updated method signature. This indicates a potential issue with the integration of metrics in the production code.

  • File: server/packs/packs.go
  • Issue: Prometheus metrics are not passed to AttachGoroutine.

Please review and ensure that the metrics are correctly integrated.

Analysis chain

Update patching function signature.

The patching function now includes Prometheus metrics and a string parameter, aligning with the updated method signature.

Ensure that the metrics are correctly integrated and used in the production code.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify integration of Prometheus metrics with the AttachGoroutine method.

# Test: Search for the AttachGoroutine method usage. Expect: Correct integration with Prometheus metrics.
rg --type go -A 5 $'AttachGoroutine'

Length of output: 2038


Script:

#!/bin/bash
# Description: Check the complete usage of AttachGoroutine in server/packs/packs.go to verify Prometheus metrics integration.

# Search for the function definition and its usage in server/packs/packs.go
rg --type go -A 10 'AttachGoroutine' server/packs/packs.go

Length of output: 308

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 09aa951 and 601eb83.

Files selected for processing (2)
  • test/integration/retention_test.go (2 hunks)
  • test/integration/snapshot_test.go (2 hunks)
Additional comments not posted (3)
test/integration/snapshot_test.go (2)

35-35: Import Prometheus metrics.

The import of Prometheus metrics indicates integration with Prometheus for monitoring.


44-49: Update patching function signature.

The patching function now includes Prometheus metrics and a string parameter, aligning with the updated method signature.

Ensure that the metrics are correctly integrated and used in the production code.

test/integration/retention_test.go (1)

39-39: Import Prometheus metrics.

The import of Prometheus metrics indicates integration with Prometheus for monitoring.

@hackerwins hackerwins self-requested a review August 14, 2024 01:56
@krapie krapie added the enhancement 🌟 New feature or request label Aug 15, 2024
@krapie krapie self-requested a review August 18, 2024 08:20
Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.
I have left some comments below :)

PS. After we merge this PR, let's add new panel in Yorkie Grafana Dashbaord using the PromQL you mentioned in your description.

server/backend/background/background.go Outdated Show resolved Hide resolved
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 601eb83 and 8870618.

Files selected for processing (3)
  • server/backend/backend.go (1 hunks)
  • server/backend/background/background.go (3 hunks)
  • server/packs/packs.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • server/backend/background/background.go
  • server/packs/packs.go
Additional comments not posted (1)
server/backend/backend.go (1)

91-91: LGTM! Verify the usage of the metrics parameter.

The introduction of the metrics parameter in the New function aligns with the PR objectives and enhances functionality.

Ensure that the metrics parameter is correctly utilized throughout the codebase and that all necessary parts of the system are updated to accommodate this change.

Verification successful

The metrics parameter is consistently utilized across the codebase.

The metrics parameter is integrated into various components, including server initialization, background tasks, and tests, ensuring that the system accommodates this change effectively.

  • Files with metrics usage:
    • server/backend/backend.go
    • server/profiling/server.go
    • server/profiling/prometheus/metrics.go
    • test/integration/housekeeping_test.go
    • test/bench/push_pull_bench_test.go

This confirms that the metrics parameter is properly utilized throughout the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the `metrics` parameter in the codebase.

# Test: Search for the usage of `metrics`. Expect: Proper usage and handling throughout the codebase.
rg --type go 'metrics'

Length of output: 2265

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 8870618 and 6c53aea.

Files selected for processing (2)
  • test/integration/retention_test.go (1 hunks)
  • test/integration/snapshot_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
  • test/integration/retention_test.go
  • test/integration/snapshot_test.go

@kokodak kokodak requested a review from krapie August 18, 2024 11:20
Copy link
Member

@krapie krapie left a comment

Choose a reason for hiding this comment

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

LGTM :)

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

@hackerwins hackerwins merged commit d73a0f0 into yorkie-team:main Aug 19, 2024
5 checks passed
hackerwins pushed a commit that referenced this pull request Aug 19, 2024
This PR adds a metric to collect the number of background routines.

Prometheus provides its own thread-safe Gauge metric, which can be
used to accurately collect the number of concurrent goroutines.

Also, although the only task running in the background so far is
PushPull, this commit implements the AttachGoroutine() method to take
a string called taskType as a parameter, since it seems to be focused
on reusability.
@hackerwins hackerwins changed the title Add metric collection for the number of goroutines attached in the Background task Add a metric to collect the number of background routines Aug 19, 2024
@krapie
Copy link
Member

krapie commented Aug 22, 2024

@kokodak We have deployed this feature with Grafana panel added.
But it seems like the metrics is not collected. Can you verify it again?

image

@kokodak
Copy link
Member Author

kokodak commented Aug 22, 2024

@krapie
There is no problem with the metric collection itself.
However, in most scenarios, the lifetime of the goroutines running in the background is extremely short, and the interval between collecting metrics is long, so it doesn't seem to be observed.

This was happening to me locally, but when I forcibly increased the lifetime of the goroutine, I was able to observe it normally.
On the yorkie main server, there is some traffic, so I thought it would be observed, but it seems not. I think the main reason is that the goroutine has a very short lifetime.

How about changing the metric from Gauge to Counter, and changing it to observe the gradient of the number of goroutines increasing in time?

@krapie
Copy link
Member

krapie commented Aug 22, 2024

@kokodak Good point! Looks like the lifespan of the goroutine is too short to collect.
Well, I can see some changes when I change the query to rate_interval.

image

cc. @hackerwins

binary-ho pushed a commit to binary-ho/yorkie that referenced this pull request Sep 1, 2024
…m#963)

This PR adds a metric to collect the number of background routines.

Prometheus provides its own thread-safe Gauge metric, which can be
used to accurately collect the number of concurrent goroutines.

Also, although the only task running in the background so far is
PushPull, this commit implements the AttachGoroutine() method to take
a string called taskType as a parameter, since it seems to be focused
on reusability.
raararaara pushed a commit that referenced this pull request Oct 7, 2024
This PR adds a metric to collect the number of background routines.

Prometheus provides its own thread-safe Gauge metric, which can be
used to accurately collect the number of concurrent goroutines.

Also, although the only task running in the background so far is
PushPull, this commit implements the AttachGoroutine() method to take
a string called taskType as a parameter, since it seems to be focused
on reusability.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🌟 New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Collect Worker Count Metric that Runs Through the Background Package
3 participants