Add NullSystemTagSet to correctly read K6_SYSTEM_TAGS from env #3660
Conversation
joanlopez
requested review from
mstoykov and
codebien
and removed request for
a team
| // NewNullSystemTagSet returns valid (Valid: true) SystemTagSet | ||
| func NewNullSystemTagSet(tags ...SystemTag) NullSystemTagSet { | ||
| return NullSystemTagSet{ | ||
| Set: NewSystemTagSet(tags...), | ||
| Valid: true, | ||
| } | ||
| } | ||
|
|
||
| // Add adds a tag to tag set. | ||
| func (n *NullSystemTagSet) Add(tag SystemTag) { | ||
| if n.Set == nil { | ||
| n.Set = new(SystemTagSet) | ||
| n.Valid = true | ||
| } | ||
| n.Set.Add(tag) | ||
| } | ||
|
|
||
| // Has checks a tag included in tag set. | ||
| func (n NullSystemTagSet) Has(tag SystemTag) bool { | ||
| if !n.Valid { | ||
| return false | ||
| } | ||
| return n.Set.Has(tag) | ||
| } | ||
|
|
||
| // Map returns the EnabledTags with current value from SystemTagSet | ||
| func (n NullSystemTagSet) Map() EnabledTags { | ||
| return n.Set.Map() | ||
| } | ||
|
|
||
| // SetString returns comma separated list of the string representation of all values in the set | ||
| func (n NullSystemTagSet) SetString() string { | ||
| return n.Set.SetString() | ||
| } |
There was a problem hiding this comment.
I take it that without this additional methods the change is much bigger ?
There was a problem hiding this comment.
I might be missing something, but some of these methods are widely used (for instance, a quick search gives ~30 occurrences of SystemTags.Has(...), so the only alternative I see is replacing each of them by: SystemTags.Set.Has(...). The same for the rest, although Has is probably the one used the most.
Do you have any other suggestion? 🤔
There was a problem hiding this comment.
Any news? Any suggestion? @mstoykov 🙏🏻
There was a problem hiding this comment.
Sorry, I started writing and then apparently never published this.
Given how big this change is and that it now mixes Null types with the actual implementation of how it is used - which we never do, I am kind of for not going forward with this.
I kind of prefer if we just use Null type and then fix it everywhere, but it might be a bit much. Arguably we will have to do this huge breaking change when we move to https://github.com/grafana/croconf or for any other fix for #883 . But not certain if we should do this before.
So at this point I kind of thing that your proposal to just patch envconfig might be better 🤔
Can you open a PR so we can at least look at it in parallel.
cc @codebien I guess this also is your concern
There was a problem hiding this comment.
Here's the PR: mstoykov/envconfig#1
cc/ @codebien
| // NullSystemTagSet is a wrapper around SystemTagSet like guregu/null | ||
| type NullSystemTagSet struct { | ||
| Set *SystemTagSet | ||
| Valid bool | ||
| } |
There was a problem hiding this comment.
In principle:
Do we need the nullable type at this level? It resolves a config/option's issue so I would prefer if this new type remains a solution for those spaces and let the rest of the code only use SystemTagSet.
In this way, the rest of the code is independent of what we need on the config side. If in the future we change the way we configure k6, and use other types for the Null problem then all the code will be impacted. Instead, if it only relies on pure SystemTagSet then we don't have to change it.
In concrete, I may miss something marking my suggestion as not feasible. @joanlopez What do you think?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3660 +/- ##
=======================================
Coverage 73.60% 73.61%
=======================================
Files 277 277
Lines 20251 20285 +34
=======================================
+ Hits 14906 14932 +26
- Misses 4399 4404 +5
- Partials 946 949 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Closing in favor of #3672 |
What?
It adds
metrics.NullSystemTagSetas a nullable wrapper ofmetrics.SystemTagSet, following the same pattern we use fromguregu/null, and with some custom types liketypes.NullHostsin order to correctly readK6_SYSTEM_TAGSfrom env.Why?
Because now
options.SystemTagsis typed as*metrics.SystemTagSet, so as the default value for pointers isnil, it cannot ever be re-assigned, causing the call toenvconfig.Process(...)to read the value set toK6_SYSTEM_TAGS, but without actually making any change to the options.Checklist
make lint) and all checks pass.make tests) and all tests pass.Related PR(s)/Issue(s)
Closes #3638