Introduce a flag that skips tag duplicate removal logic #165
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -148,7 +154,7 @@ func (eng *Engine) measure(t time.Time, name string, value interface{}, ftype Fi | |||
| m.Tags = append(m.Tags[:0], eng.Tags...) | |||
| m.Tags = append(m.Tags, tags...) | |||
|
|
|||
| if len(tags) != 0 && !TagsAreSorted(m.Tags) { | |||
| if len(tags) != 0 && !eng.SkipTagDuplicateRemoval && !TagsAreSorted(m.Tags) { | |||
| SortTags(m.Tags) | |||
There was a problem hiding this comment.
Do we still want to sort tags even if we are not removing duplicates?
There was a problem hiding this comment.
Can we rename the SortTags function to RemoveDuplicates that's really what it's doing, the sorting is an artifact of that.
There was a problem hiding this comment.
It makes sense but I can see other usages such as flagon. Do you want to rename to RemoveDuplicates because it doesn't offer the backwards compatibility?
|
@kevinburkesegment would you please review again? Thank you! |
|
@kevinburkesegment I just wanted to follow up. Could you please review this PR when you have a moment? Thank you so much! |
kevinburkesegment
force-pushed
the
skip-tag-duplicate-removal
branch
2 times, most recently
from
dc46363
to
f03f0bc
Compare
kevinburkesegment
force-pushed
the
skip-tag-duplicate-removal
branch
from
f03f0bc
to
ec8ad7d
Compare
kevinburkesegment
approved these changes
Jul 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request primarily introduces a new feature to the
Enginestruct inengine.gowhich allows the skipping of duplicate tag removal in the metrics reporting process. This feature is tested inengine_test.go.The most important changes are:
New feature:
engine.go: Added a new boolean fieldSkipTagDuplicateRemovalto theEnginestruct. This field controls whether the engine should skip the removal of duplicate tags from the tags list before sending.Feature integration:
engine.go: Modified themeasureandReportAtmethods of theEnginestruct to check theSkipTagDuplicateRemovalfield before removing duplicate tags. [1] [2]Testing:
engine_test.go: Added a new test scenario inTestEngineto test the behavior whenSkipTagDuplicateRemovalis set.engine_test.go: Implemented a new test functiontestEngineSkipTagDuplicateRemovalto test the new feature.Contexts
https://twilio.slack.com/archives/CPLTV1V9A/p1720735336325819