feat(metrics): be able to send new tags on stop functions #7
Conversation
WalkthroughThe changes in this pull request primarily modify the Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- metrics.go (6 hunks)
- metrics_test.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (9)
metrics_test.go (6)
46-46: Correct inclusion of additional tags in deferred functionThe deferred call to
ReportFuncCallAndTimingWithErrnow includes an additionalTagsparameterTags{"stop": "error"}, which correctly tests the new functionality of passing additional tags when stopping functions.
58-58: Proper use ofassert.ElementsMatchfor tag comparisonThe test assertions have been updated to use
assert.ElementsMatchinstead ofassert.Equalwhen comparing tags. This is appropriate because the order of tags may not be guaranteed, andassert.ElementsMatchcorrectly verifies that all expected tags are present regardless of order.Also applies to: 63-63, 67-67
60-60: Updated expected tags to include new stop tagThe
expectedTagsslice has been updated to include the new tag"stop=error", ensuring that the test correctly validates the presence of all expected tags.
84-98: New test case added for deferred function with new tagsA new test case
t.Run("can be deferred with new tags and skip error reporting", ...)has been added to verify that additional tags can be passed when deferringReportFuncCallAndTimingWithErr, and that error reporting is skipped whenerrisnil.
88-88: Ensure additional tags are correctly handled in deferred callIn the deferred call,
ReportFuncCallAndTimingWithErris called withTags{"foo": "bar"}and an additionalTags{"something": "new"}when stopping. This correctly tests the ability to pass extra tags upon stopping.
94-97: Validate expected tags in assertionsThe
expectedTagsslice includes the new tag"something=new", and the assertions correctly useassert.ElementsMatchto verify that all expected tags are present in the recorded calls.metrics.go (3)
173-181: Ensure correct handling of additional stop tags in timing functionsIn the anonymous function returned by
reportTiming, thestopTagsare now accepted as a parameter and appended totagArray. Please confirm that this modification correctly accumulates the intended tags and does not introduce any unintended side effects.
220-220: Appending stop tags inReportClosureFuncTimingAppending
stopTagstotagArrayinReportClosureFuncTimingchanges the reported tags. Ensure that this change aligns with the intended behavior and that all tags are correctly merged.
86-93:⚠️ Potential issuePotential backward compatibility issue due to function signature change
The function
ReportFuncCallAndTimingWithErrhas been modified to return a function with a new signature:func(err *error, tags ...Tags)instead offunc(err *error). This change can break existing code that uses this function, as the returned function's signature has changed.To ensure backward compatibility, please verify if any existing callers depend on the previous function signature.
✅ Verification successful
Backward compatibility maintained for
ReportFuncCallAndTimingWithErrNo compatibility issues found with existing callers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of `ReportFuncCallAndTimingWithErr` to check for compatibility issues rg --type go 'ReportFuncCallAndTimingWithErr' --threads 4Length of output: 446
| @@ -111,7 +111,7 @@ func reportFunc(fn, action string, tags ...Tags) { | |||
| client.Incr(fmt.Sprintf("func.%v", action), tagArray, 0.77) | |||
| } | |||
|
|
|||
| type StopTimerFunc func() | |||
| type StopTimerFunc func(tags ...Tags) | |||
There was a problem hiding this comment.
Changing StopTimerFunc type may break existing implementations
The type StopTimerFunc has been updated from func() to func(tags ...Tags). This modification can affect all existing implementations and usages of this type, potentially causing compatibility issues.
| @@ -127,7 +127,7 @@ func reportTiming(ctx context.Context, tags ...Tags) (context.Context, StopTimer | |||
| defer clientMux.RUnlock() | |||
|
|
|||
| if client == nil { | |||
| return ctx, func() {} | |||
| return ctx, func(...Tags) {} | |||
There was a problem hiding this comment.
Anonymous function signature changed to accept ...Tags
The anonymous function returned now accepts variadic ...Tags as parameters. This change modifies the function signature from func() to func(...Tags), which may disrupt existing code expecting a parameterless function.
| @@ -187,7 +189,7 @@ func ReportClosureFuncTiming(name string, tags ...Tags) StopTimerFunc { | |||
| clientMux.RLock() | |||
| defer clientMux.RUnlock() | |||
| if client == nil { | |||
| return func() {} | |||
| return func(...Tags) {} | |||
There was a problem hiding this comment.
Change in returned function's signature may affect callers
The returned function now accepts ...Tags as parameters, changing its signature from func() to func(...Tags). Verify that any callers of this function are updated to accommodate this change.
This PR enables sending more tags on Stop functions, enabling us to send metadata calculated during the functions
This change is backwards compatible and should not break any call
Summary by CodeRabbit
New Features
Bug Fixes
Tests