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

chore: defining bucket boundaries #4744

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Conversation

achettyiitr
Copy link
Member

@achettyiitr achettyiitr commented Jun 2, 2024

Description

  • Defining explicit bucket boundaries for HistogramType and TimerType.
  • I have used this dashboard for deciding the bucket sizes and this for deciding what metrics to target at an initial level.

Linear Ticket

  • Resolves PIPE-1079
  • Resolves PIPE-1080

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

Summary by CodeRabbit

  • Refactor
    • Updated histogram bucket configurations for various metrics to provide more precise data analysis.
    • Removed global variables for default histogram buckets and implemented dynamic assignment based on conditions.

Copy link
Contributor

@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 d3cc894 and 47beb87.

Files selected for processing (3)
  • integration_test/kafka_batching/kafka_batching_test.go (2 hunks)
  • runner/buckets.go (1 hunks)
  • runner/runner.go (2 hunks)
Additional comments not posted (4)
runner/buckets.go (2)

15-24: Updated bucket configurations for event_delivery_time in both server and warehouse contexts.

Ensure that these new configurations are reflected in all relevant metrics calculations and visualizations.


26-99: Significant updates to customBuckets with new metrics and updated bucket sizes.

Verify that these new bucket sizes are appropriate for the metrics they are intended to measure, considering the expected range of values.

runner/runner.go (1)

113-115: Dynamic assignment of histogram buckets based on warehouse mode.

This change allows for more flexible configuration of metrics based on operational mode, which can be beneficial for performance tuning and resource management.

integration_test/kafka_batching/kafka_batching_test.go (1)

287-302: Updated histogram buckets in test assertions to match new configurations.

Ensure that these new bucket sizes are tested under various load scenarios to confirm that they capture the metric accurately across expected operational ranges.

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.70%. Comparing base (d3cc894) to head (79cdae9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4744      +/-   ##
==========================================
+ Coverage   74.62%   74.70%   +0.08%     
==========================================
  Files         414      414              
  Lines       48591    48593       +2     
==========================================
+ Hits        36259    36300      +41     
+ Misses       9956     9921      -35     
+ Partials     2376     2372       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rudderlabs rudderlabs deleted a comment from coderabbitai bot Jun 3, 2024
Copy link
Contributor

@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: 5

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d3cc894 and 97543e9.

Files selected for processing (3)
  • integration_test/kafka_batching/kafka_batching_test.go (2 hunks)
  • runner/buckets.go (1 hunks)
  • runner/runner.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • integration_test/kafka_batching/kafka_batching_test.go
  • runner/runner.go
Additional context used
golangci-lint
runner/buckets.go

8-8: var defaultHistogramBuckets is unused


11-11: var defaultWarehouseHistogramBuckets is unused


15-15: var customBucketsServer is unused


20-20: var customBucketsWarehouse is unused


26-26: var customBuckets is unused

@rudderlabs rudderlabs deleted a comment from coderabbitai bot Jun 3, 2024
@rudderlabs rudderlabs deleted a comment from coderabbitai bot Jun 3, 2024
@rudderlabs rudderlabs deleted a comment from coderabbitai bot Jun 3, 2024
@rudderlabs rudderlabs deleted a comment from coderabbitai bot Jun 3, 2024
@rudderlabs rudderlabs deleted a comment from coderabbitai bot Jun 3, 2024
@rudderlabs rudderlabs deleted a comment from coderabbitai bot Jun 3, 2024
@achettyiitr achettyiitr changed the title chore: bucket boundaries chore: defining bucket boundaries Jun 3, 2024
@rudderlabs rudderlabs deleted a comment from coderabbitai bot Jun 4, 2024
Copy link
Contributor

@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 d3cc894 and 79cdae9.

Files selected for processing (3)
  • integration_test/kafka_batching/kafka_batching_test.go (2 hunks)
  • runner/buckets.go (1 hunks)
  • runner/runner.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • integration_test/kafka_batching/kafka_batching_test.go
  • runner/runner.go
Additional comments not posted (5)
runner/buckets.go (5)

8-10: Updated default histogram bucket values.

This change updates the default histogram buckets to a new set of values. Ensure that all dependent metrics and systems are compatible with these new bucket sizes.


11-13: Updated default warehouse histogram bucket values.

This change updates the default warehouse histogram buckets to a new set of values. Verify that all warehouse metrics are compatible with these new bucket sizes.


15-19: Updated custom server bucket values for event_delivery_time.

This update extends the range of bucket sizes for event_delivery_time to cover up to 24 hours. Ensure that this aligns with the expected data distribution and usage patterns.


20-24: Updated custom warehouse bucket values for event_delivery_time.

This update modifies the bucket sizes for event_delivery_time in the warehouse context to include more granular time intervals. Verify that this change matches the expected data granularity and reporting needs.


26-103: Extensive updates to custom bucket configurations.

The custom bucket configurations have been extensively updated across various metrics. It's crucial to ensure that these changes are reflected in all relevant dashboards and monitoring systems to maintain accuracy in metrics reporting.

@rudderlabs rudderlabs deleted a comment from coderabbitai bot Jun 4, 2024
@fracasula fracasula merged commit 4568777 into master Jun 4, 2024
54 checks passed
@fracasula fracasula deleted the chore.bucket-boundaries branch June 4, 2024 15:43
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.

4 participants