-
Notifications
You must be signed in to change notification settings - Fork 39
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 experimental support for APM latency with Exp Histogram #288
Conversation
WalkthroughThe recent changes introduce support for exponential histograms in the span metrics processor. A new configuration option allows users to enable this feature, which is backed by the implementation of a new exponential histogram type, updates to the processor to handle these histograms, and functions to manage and collect histogram metrics. This enhancement aims to improve metric precision and efficiency in representing distribution data. Changes
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Additionally, you can add CodeRabbit Configration File (
|
There is no apples-to-apples comparison since one has 10x more buckets yet It is roughly ~8x faster with better accuracy (relative error no greater than 4% in worst case) on sample data. But the production data is the real ground to verify the numbers. This is disabled by default and I will enable this for select customers and do some analysis. If there are no concerns, I would like to see this get merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I am only reviewing the syntactical things as I don't have the expertise. Since you want this to test this on customer instance I have no concerns as of now.
Also would be great if you can do a small session on exp histogram 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- processor/signozspanmetricsprocessor/config.go (1 hunks)
- processor/signozspanmetricsprocessor/factory.go (1 hunks)
- processor/signozspanmetricsprocessor/processor.go (17 hunks)
Additional comments: 19
processor/signozspanmetricsprocessor/factory.go (1)
- 51-51: The addition of
EnableExpHistogram
with a default value offalse
is aligned with the PR objectives and best practices for introducing experimental features.processor/signozspanmetricsprocessor/config.go (1)
- 87-87: The addition of the
EnableExpHistogram
boolean field to theConfig
struct is necessary for the feature's configuration and follows good coding practices.processor/signozspanmetricsprocessor/processor.go (17)
- 71-73: The
exponentialHistogram
struct is introduced correctly with a field for the histogram. Ensure that thestructure.Histogram[float64]
type from thego-expohisto
library is the most suitable choice for the intended use case.- 88-88: Adding
expDimensions
to theprocessorImp
struct is appropriate for handling dimensions specific to exponential histograms. This aligns with the existing design pattern for handling dimensions.- 100-100: The introduction of
expHistograms
map in theprocessorImp
struct is consistent with the existing structure for handling histograms. This is necessary for storing exponential histogram data.- 116-116: The addition of
expHistogramKeyToDimensions
cache is consistent with the design pattern of using caches for dimension mapping. This is crucial for performance optimization when handling exponential histograms.- 158-185: The function
expoHistToExponentialDataPoint
correctly converts an exponential histogram to an exponential data point. However, ensure that the handling of positive and negative buckets is correctly implemented according to the expected behavior of thestructure.Histogram
library.- 187-189: The
Observe
method in theexponentialHistogram
struct correctly updates the histogram with a new value. This is a standard approach for histogram observation.- 230-233: Initializing the
expHistogramKeyToDimensionsCache
with the configured cache size is done correctly. This ensures that the cache size is configurable and aligns with the existing pattern for other caches.- 259-259: Creating the
expHistograms
map during processor initialization is necessary for storing exponential histogram data. This aligns with the initialization of other histogram maps.- 268-268: Adding
expDimensions
during processor initialization ensures that exponential histogram-specific dimensions are correctly handled. This is consistent with the handling of other dimensions.- 274-274: Initializing the
expHistogramKeyToDimensions
cache during processor setup is done correctly. This ensures that the cache is ready for use when processing spans.- 484-487: The
collectExpHistogramMetrics
function is correctly implemented to collect and write exponential histogram metrics. Ensure that the metrics are correctly aggregated and reported according to the configured aggregation temporality.- 515-519: The logic to delete keys from
expHistograms
that are no longer present in theexpHistogramKeyToDimensions
cache is correctly implemented. This helps in managing memory usage effectively.- 571-594: The implementation of
collectExpHistogramMetrics
for collecting and writing exponential histogram metrics is correct. Ensure that the data points are correctly populated and that the dimensions are accurately copied to the metrics.- 714-720: The
getDimensionsByExpHistogramKey
function correctly retrieves dimensions from theexpHistogramKeyToDimensions
cache. This is consistent with the pattern used for other dimension retrieval functions.- 889-895: The conditional logic to handle exponential histograms based on the
EnableExpHistogram
configuration is correctly implemented. This ensures that the feature is opt-in and does not affect existing functionality unless explicitly enabled.- 958-964: Resetting the
expHistograms
map and purging theexpHistogramKeyToDimensions
cache during the reset of accumulated metrics is correctly implemented. This is necessary for correctly handling delta metrics and managing memory usage.- 988-1004: The
updateExpHistogram
function correctly updates the exponential histogram with a new latency value. Ensure that the histogram configuration (e.g., max size) is appropriately set for the intended use case.
Follow up #279 (comment). This is more accurate (i.e. as close to latency from traces) and faster.
The query would change
From
To
Summary by CodeRabbit