Refactoring lib.TagSet #1148

cuonglm · 2019-09-09T14:48:48Z

This PR introduce new SystemTagSet, SystemTagMap under stats package.

SystemTagMap is the same as old lib.TagSet.
SystemTagSet is a bit mask.

Putting them under stats package is more appropriate and have better semantic, also it help reducing complexity of current lib package and prevent import cycle.

Old lib.TagSet is removed.

Close #755

lib/tagset/tagset.go

codecov-io · 2019-09-09T15:16:11Z

Codecov Report

Merging #1148 into master will decrease coverage by 0.03%.
The diff coverage is 70.94%.

@@            Coverage Diff             @@
##           master    #1148      +/-   ##
==========================================
- Coverage   73.67%   73.63%   -0.04%     
==========================================
  Files         145      147       +2     
  Lines       10604    10639      +35     
==========================================
+ Hits         7812     7834      +22     
- Misses       2333     2345      +12     
- Partials      459      460       +1

Impacted Files	Coverage Δ
lib/options.go	`93.14% <ø> (+0.1%)`	⬆️
stats/dummy/collector.go	`60% <0%> (ø)`	⬆️
stats/influxdb/collector.go	`76.85% <0%> (ø)`	⬆️
stats/kafka/collector.go	`50.68% <0%> (ø)`	⬆️
stats/cloud/collector.go	`71.38% <0%> (ø)`	⬆️
stats/json/collector.go	`12.34% <0%> (ø)`	⬆️
cmd/collectors.go	`0% <0%> (ø)`	⬆️
js/modules/k6/ws/ws.go	`81.3% <100%> (ø)`	⬆️
stats/csv/collector.go	`83.78% <100%> (ø)`	⬆️
cmd/options.go	`67.76% <100%> (ø)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce85f80...b0bd035. Read the comment docs.

lib/tagset/tagset.go

js/modules/k6/ws/ws_test.go

js/modules/k6/http/request_test.go

cmd/config_consolidation_test.go

stats/system_tag.go

stats/system_tag_test.go

stats/system_tag.go

stats/system_tag_set.go

stats/datadog/collector_test.go

stats/system_tag.go

lib/options.go

cmd/options.go

js/modules/k6/ws/ws_test.go

stats/system_tag.go

stats/datadog/collector.go

stats/influxdb/collector.go

stats/system_tag_set.go

cmd/options.go

stats/system_tag.go

stats/datadog/collector.go

stats/datadog/collector_test.go

js/modules/k6/ws/ws_test.go

na--

LGTM, thanks for patiently working through all of the reviews!

mstoykov

LGTM but if you can just change to use map[string]struct{} instead of map[string]bool

stats/system_tag.go

@imiric

Add tag set as a bitmask, as part of refactoring lib.TagSet. Update #755 silent linter fix @imiric's review comments

fix golangci-lint

The map variant is meant to be used when there's a mix between system and non-system tags, it doesn't make sense to contain System in the name.

…a#1219)

cuonglm requested review from mstoykov, imiric and na-- September 9, 2019 14:48

golangcibot reviewed Sep 9, 2019

View reviewed changes

lib/tagset/tagset.go Outdated Show resolved Hide resolved

lib/tagset/tagset.go Outdated Show resolved Hide resolved

lib/tagset/tagset.go Outdated Show resolved Hide resolved

lib/tagset/tagset.go Outdated Show resolved Hide resolved

imiric suggested changes Sep 10, 2019

View reviewed changes

lib/tagset/tagset.go Outdated Show resolved Hide resolved

lib/tagset/tagset.go Outdated Show resolved Hide resolved

lib/tagset/tagset.go Outdated Show resolved Hide resolved

lib/tagset/tagset.go Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

na-- changed the title ~~lib/tagset: add tag set as bitmask~~ [WIP] lib/tagset: add tag set as bitmask Sep 13, 2019

na-- added this to the v0.26.0 milestone Sep 13, 2019

golangcibot reviewed Sep 16, 2019

View reviewed changes

cuonglm changed the title ~~[WIP] lib/tagset: add tag set as bitmask~~ lib/tagset: add tag set as bitmask Sep 18, 2019

imiric previously approved these changes Sep 24, 2019

View reviewed changes

cuonglm changed the title ~~lib/tagset: add tag set as bitmask~~ Refactoring lib.TagSet Sep 25, 2019

na-- requested changes Sep 26, 2019

View reviewed changes

stats/system_tag.go Outdated Show resolved Hide resolved

stats/system_tag.go Show resolved Hide resolved

stats/system_tag.go Outdated Show resolved Hide resolved

cuonglm dismissed imiric’s stale review via ea613a7 September 27, 2019 03:30

cuonglm force-pushed the feature/755 branch from 969ea64 to ea613a7 Compare September 27, 2019 03:30

golangcibot reviewed Sep 27, 2019

View reviewed changes

stats/system_tag_test.go Outdated Show resolved Hide resolved

stats/system_tag_test.go Outdated Show resolved Hide resolved

stats/system_tag.go Outdated Show resolved Hide resolved

cuonglm force-pushed the feature/755 branch from ea613a7 to 0330f8d Compare September 27, 2019 03:38

cuonglm requested review from imiric and na-- September 27, 2019 03:41

cuonglm force-pushed the feature/755 branch 2 times, most recently from 514bbc9 to 7d735e2 Compare September 30, 2019 05:28

golangcibot reviewed Sep 30, 2019

View reviewed changes

stats/system_tag_set.go Outdated Show resolved Hide resolved

stats/system_tag_set.go Outdated Show resolved Hide resolved

stats/system_tag_set.go Outdated Show resolved Hide resolved

stats/system_tag_set.go Outdated Show resolved Hide resolved

cuonglm force-pushed the feature/755 branch from 7d735e2 to 6ec23ab Compare September 30, 2019 05:34

mstoykov reviewed Sep 30, 2019

View reviewed changes

stats/datadog/collector_test.go Outdated Show resolved Hide resolved

na-- requested changes Oct 14, 2019

View reviewed changes

cuonglm requested review from na-- and mstoykov October 14, 2019 15:59

na-- requested changes Oct 15, 2019

View reviewed changes

na-- mentioned this pull request Oct 15, 2019

Configuration issues #883

Open

na-- previously approved these changes Oct 17, 2019

View reviewed changes

mstoykov requested changes Oct 17, 2019

View reviewed changes

stats/system_tag.go Show resolved Hide resolved

mstoykov approved these changes Oct 17, 2019

View reviewed changes

mstoykov previously approved these changes Oct 17, 2019

View reviewed changes

cuonglm added 12 commits October 17, 2019 19:48

lib/tagset: add tag set as bitmask

4e554c9

Add tag set as a bitmask, as part of refactoring lib.TagSet. Update #755 silent linter fix @imiric's review comments

move system tag set to stats package

42533f4

stats: use Tag prefix for stats.SystemTagSet

7601812

stats: add SystemTagMap

5b896e2

all: switch to new SystemTagSet/Map

5f6ef7b

fix golangci-lint

stats: move DefaultSystemTagList after SystemTag definition

e3bd67c

stats: modify code generated from enumer

3e3dc35

all: change default system tag to SystemTagSet

ad0ee95

stats: SystemTagMap -> TagSet

05cdbee

The map variant is meant to be used when there's a mix between system and non-system tags, it doesn't make sense to contain System in the name.

stats/datadog: use TagSet instead of SystemTagSet

cee1977

Add NewSystemTagSet

95ab3b2

use enumer to generate

5c82b54

cuonglm dismissed stale reviews from mstoykov and na-- via 5c82b54 October 17, 2019 12:49

cuonglm force-pushed the feature/755 branch from b0bd035 to 5c82b54 Compare October 17, 2019 12:49

cuonglm requested review from na-- and mstoykov October 17, 2019 12:50

na-- approved these changes Oct 17, 2019

View reviewed changes

mstoykov approved these changes Oct 17, 2019

View reviewed changes

cuonglm merged commit 4b0cf7f into master Oct 17, 2019

cuonglm deleted the feature/755 branch October 17, 2019 13:13

mstoykov added a commit that referenced this pull request Oct 28, 2019

Fix the system-tag cli help default values after #1148

8313bd8

mstoykov added a commit that referenced this pull request Oct 29, 2019

Fix the system-tag cli help default values after #1148 (#1219)

9e1c5c1

mstoykov mentioned this pull request Oct 29, 2019

Fix the system-tag cli help default values #1219

Merged

srguglielmo pushed a commit to srguglielmo/k6 that referenced this pull request Nov 3, 2019

Fix the system-tag cli help default values after grafana#1148 (grafan…

c048bcf

…a#1219)

mstoykov mentioned this pull request Aug 24, 2020

K6_DATADOG_TAG_BLACKLIST not functional #1602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring lib.TagSet #1148

Refactoring lib.TagSet #1148

cuonglm commented Sep 9, 2019 •

edited

Loading

codecov-io commented Sep 9, 2019 •

edited

Loading

This comment has been minimized.

na-- left a comment

mstoykov left a comment

Refactoring lib.TagSet #1148

Refactoring lib.TagSet #1148

Conversation

cuonglm commented Sep 9, 2019 • edited Loading

codecov-io commented Sep 9, 2019 • edited Loading

Codecov Report

This comment has been minimized.

na-- left a comment

Choose a reason for hiding this comment

mstoykov left a comment

Choose a reason for hiding this comment

cuonglm commented Sep 9, 2019 •

edited

Loading

codecov-io commented Sep 9, 2019 •

edited

Loading