feat(GEN-95|HELIX-1134): add datadog and mixpanel support #6
Conversation
feat(HELIX-1134): add mixpanel support
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduced in this pull request enhance the metrics handling functionality by adding MixPanel integration and expanding profiling capabilities. New configuration options have been added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Metrics
participant MixPanel
participant Profiler
Client->>Metrics: Initialize Metrics Client
Metrics->>MixPanel: Start MixPanel Client
Metrics->>Profiler: Setup Profiler
Client->>Metrics: Report Event
Metrics->>MixPanel: Track Event
Metrics->>Profiler: Record Profile Data
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (4)
- client.go (6 hunks)
- go.mod (6 hunks)
- metrics.go (5 hunks)
- reports.go (2 hunks)
Additional context used
golangci-lint
reports.go
63-63: Error return value of
s.Timingis not checked(errcheck)
69-69: Error return value of
s.Gaugeis not checked(errcheck)
Additional comments not posted (25)
reports.go (5)
45-49: LGTM!The
Counterfunction is implemented correctly and uses theCustomReportfunction to handle the reporting logic.
51-55: LGTM!The
CounterPositivefunction is implemented correctly and uses generics to accept any integer type for the value argument. It conditionally calls theCounterfunction only if the value is greater than zero, ensuring that only positive counts are recorded.
57-59: LGTM!The
Incrfunction is implemented correctly and simplifies the process of incrementing a metric by one by calling theCounterfunction with a value of 1.
73-78: LGTM!The
BoolTagfunction is implemented correctly and provides a convenient way to convert boolean values to string representations suitable for tagging.
80-82: LGTM!The
IntTagfunction is implemented correctly and uses generics to accept any integer type. It converts the integer value to a string representation usingstrconv.Itoa.go.mod (5)
7-7: LGTM!The addition of the
github.com/InjectiveLabs/suplogdependency at versionv1.3.3looks good. It will likely enhance the logging capabilities of the project.
10-10: LGTM!The addition of the
github.com/mixpanel/mixpanel-godependency at versionv1.2.1looks good. It will enable Mixpanel analytics integration in the project.
38-38: LGTM!The addition of the
github.com/aws/aws-sdk-goindirect dependency at versionv1.44.327looks good. It's likely pulled in transitively by another direct dependency and should not cause any issues.
42-43: LGTM!The addition of the
github.com/bugsnag/bugsnag-goandgithub.com/bugsnag/panicwrapindirect dependencies at versionsv1.5.3andv1.3.4respectively looks good. They are likely pulled in transitively by another direct dependency and should not cause any issues.
112-112: LGTM!The addition of the
github.com/sirupsen/logrusindirect dependency at versionv1.9.3looks good. It's likely pulled in transitively by another direct dependency and should not cause any issues.metrics.go (5)
27-34: LGTM!The
ReportFuncDeferredErrorfunction provides a useful utility for deferred error reporting. It captures the caller function name and returns a closure that checks for errors and reports them usingReportClosureFuncError. This can help in identifying the source of errors and ensure that they are not missed.
74-84: LGTM!The
ReportFuncCallAndTimingCtxWithErrfunction is a useful addition that combines timing and error reporting in a context-aware manner. It starts a timer usingreportTimingand returns a closure that stops the timer and reports any errors usingReportClosureFuncError. This can help in measuring the execution time of functions and identifying performance bottlenecks and error-prone functions.
230-239: LGTM!The
Trackfunction provides a simple and convenient way to send events to Mixpanel. It checks if themixPanelClientis initialized and sends the events usingmixPanelClient.Trackwith the provided context. If an error occurs during sending, it returns the error. If the client is not initialized or the sending is successful, it returns nil. This function is context-aware and handles the case when the Mixpanel client is not initialized gracefully.
241-247: LGTM!The
NewEventfunction provides a simple way to create new Mixpanel events. It checks if themixPanelClientis initialized and creates a new event usingmixPanelClient.NewEventwith the provided event name, distinct ID, and properties. If the client is not initialized, it returns nil. This function handles the case when the Mixpanel client is not initialized gracefully.
268-279: LGTM!The
MergeTagsfunction provides a useful utility for merging multiple tag maps into a single map. It creates a new tag map, copies the key-value pairs from the original map, and then iterates over the source tag maps, copying their key-value pairs into the destination map. The merging process overwrites existing keys with the values from the source maps, following a last-write-wins strategy. This function allows for easy combination of tags from different sources.client.go (10)
4-15: LGTM!The new imports for Mixpanel and Datadog profiler are valid and necessary for the added functionality.
32-34: LGTM!The new variables for the Datadog tracer provider, tracer, and Mixpanel client are properly initialized and necessary for the added functionality.
43-51: LGTM!The new fields added to the
StatterConfigstruct for version, mocking threshold, profiling, and Mixpanel are properly defined and provide necessary configuration options for the added functionality.
103-103: LGTM!Using the
cfgparameter directly in thenewMockStatterfunction call is more appropriate and improves the function's independence and testability.
143-158: LGTM!The new code for initializing the Datadog tracer provider and tracer, setting up the profiler, and starting Mixpanel is properly guarded by the respective configuration flags and includes appropriate error handling.
163-167: LGTM!The new
StartMixPanelfunction properly initializes the Mixpanel client using the providedprojectTokenand ensures thread safety with a mutex lock.
169-187: LGTM!The new
setupProfilerfunction properly configures the Datadog profiler with the provided configuration options, sets the mutex and block profile rates, and includes appropriate error handling and wrapping.
205-205: LGTM!Using
log.WithErrorin theerrHandlerfunction is a more structured and consistent way of logging errors, improving the readability of error messages.
208-214: LGTM!The updates to the
newMockStatterfunction to take thecfgparameter directly and use theMockingThresholdfield are properly implemented and improve the function's independence and configurability.
218-290: LGTM!The updates to the
mockStatterstruct and its methods to use thethresholdfield for conditional logging are consistently applied and properly implemented, improving the configurability of the mock statter.
| func Timer(metric string, value time.Duration, tags ...Tags) { | ||
| CustomReport(func(s Statter, tagSpec []string) { | ||
| s.Timing(metric, value, tagSpec, 1) | ||
| }, tags...) | ||
| } |
There was a problem hiding this comment.
Handle the error return value of s.Timing.
The Timer function is implemented correctly, but it ignores the error return value of s.Timing. This could lead to silent failures if the timing metric fails to be recorded.
Consider handling the error return value to ensure that any failures are logged or propagated appropriately. For example:
func Timer(metric string, value time.Duration, tags ...Tags) {
CustomReport(func(s Statter, tagSpec []string) {
if err := s.Timing(metric, value, tagSpec, 1); err != nil {
// Handle the error, e.g., log it or propagate it
log.Printf("Failed to record timing metric: %v", err)
}
}, tags...)
}Tools
golangci-lint
63-63: Error return value of
s.Timingis not checked(errcheck)
| func Gauge(metric string, value float64, tags ...Tags) { | ||
| CustomReport(func(s Statter, tagSpec []string) { | ||
| s.Gauge(metric, value, tagSpec, 1) | ||
| }, tags...) | ||
| } |
There was a problem hiding this comment.
Handle the error return value of s.Gauge.
The Gauge function is implemented correctly, but it ignores the error return value of s.Gauge. This could lead to silent failures if the gauge metric fails to be recorded.
Consider handling the error return value to ensure that any failures are logged or propagated appropriately. For example:
func Gauge(metric string, value float64, tags ...Tags) {
CustomReport(func(s Statter, tagSpec []string) {
if err := s.Gauge(metric, value, tagSpec, 1); err != nil {
// Handle the error, e.g., log it or propagate it
log.Printf("Failed to record gauge metric: %v", err)
}
}, tags...)
}Tools
golangci-lint
69-69: Error return value of
s.Gaugeis not checked(errcheck)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
reports.go (1)
74-78: SimplifyBoolTagusingstrconv.FormatBool.You can simplify the
BoolTagfunction by using the standard library functionstrconv.FormatBool, which returns the string representation of a boolean value.Apply this diff to streamline the function:
func BoolTag(b bool) string { - if b { - return "true" - } - return "false" + return strconv.FormatBool(b) }
| func Counter[T constraints.Integer](metric string, value T, tags ...Tags) { | ||
| CustomReport(func(s Statter, tagSpec []string) { | ||
| s.Count(metric, int64(value), tagSpec, 1) | ||
| }, tags...) | ||
| } |
There was a problem hiding this comment.
Handle the error return value of s.Count in Counter.
The s.Count method returns an error that is currently being ignored. This could lead to silent failures if the count metric fails to be recorded. It's important to handle or log the error to ensure reliability.
Apply this diff to handle the error:
func Counter[T constraints.Integer](metric string, value T, tags ...Tags) {
CustomReport(func(s Statter, tagSpec []string) {
if err := s.Count(metric, int64(value), tagSpec, 1); err != nil {
// Handle the error, e.g., log it or propagate it
log.Printf("Failed to record count metric: %v", err)
}
}, tags...)
}| func IntTag[T constraints.Integer](i T) string { | ||
| return strconv.Itoa(int(i)) | ||
| } |
There was a problem hiding this comment.
Avoid casting to int in IntTag to prevent data loss.
Casting i to int with int(i) may lead to data loss or overflow when i is of a larger integer type (e.g., int64, uint64) that exceeds the range of int. This can cause incorrect tag values for metrics.
Consider using fmt.Sprint to safely convert any integer type to its string representation without casting:
-import (
- "strconv"
-)
+import (
+ "fmt"
+)
func IntTag[T constraints.Integer](i T) string {
- return strconv.Itoa(int(i))
+ return fmt.Sprint(i)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func IntTag[T constraints.Integer](i T) string { | |
| return strconv.Itoa(int(i)) | |
| } | |
| import ( | |
| "fmt" | |
| ) | |
| func IntTag[T constraints.Integer](i T) string { | |
| return fmt.Sprint(i) | |
| } |
| func CounterPositive[T constraints.Integer](metric string, value T, tags ...Tags) { | ||
| if value > 0 { | ||
| Counter(metric, value, tags...) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure correct comparison in CounterPositive for unsigned integers.
The condition value > 0 may not behave as expected for unsigned integer types due to their nature of never being negative. While it works correctly, consider explicitly handling unsigned integers to avoid confusion.
Specify that T is a signed integer to make the intention clear:
-func CounterPositive[T constraints.Integer](metric string, value T, tags ...Tags) {
+func CounterPositive[T constraints.Signed](metric string, value T, tags ...Tags) {
if value > 0 {
Counter(metric, value, tags...)
}
}Alternatively, clarify the intent in comments or ensure that the function is used appropriately with signed integers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func CounterPositive[T constraints.Integer](metric string, value T, tags ...Tags) { | |
| if value > 0 { | |
| Counter(metric, value, tags...) | |
| } | |
| } | |
| func CounterPositive[T constraints.Signed](metric string, value T, tags ...Tags) { | |
| if value > 0 { | |
| Counter(metric, value, tags...) | |
| } | |
| } |
adds support for datadog (Profiles and Spans)
adds mixpanel support (BAU metrics)
this PR also merges the indexer client of metrics in here!
https://github.com/InjectiveLabs/injective-indexer/blob/dev/metrics/client.go
Summary by CodeRabbit
New Features
Bug Fixes
Chores