Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the system-tag cli help default values #1219

Merged
merged 4 commits into from
Oct 29, 2019
Merged

Fix the system-tag cli help default values #1219

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8313bd8
Fix the system-tag cli help default values after #1148
mstoykov Oct 28, 2019
bea5352
Typo and optimization
mstoykov Oct 28, 2019
86916d0
Don't sort the keys in SetString as this was added because of maps be…
mstoykov Oct 28, 2019
3dae038
typo
mstoykov Oct 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ func optionFlagSet() *pflag.FlagSet {
// system-tags must have a default value, but we can't specify it here, otherwiese, it will always override others.
// set it to nil here, and add the default in applyDefault() instead.
systemTagsCliHelpText := fmt.Sprintf(
"only include these system tags in metrics (default %s)",
stats.DefaultSystemTagSet,
"only include these system tags in metrics (default %q)",
stats.DefaultSystemTagSet.SetString(),
)
flags.StringSlice("system-tags", nil, systemTagsCliHelpText)
flags.StringSlice("tag", nil, "add a `tag` to be applied to all samples, as `[name]=[value]`")
Expand Down
14 changes: 13 additions & 1 deletion stats/system_tag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package stats
import (
"bytes"
"encoding/json"
"sort"
"strings"
)

Expand Down Expand Up @@ -60,7 +61,7 @@ func (i *SystemTagSet) Has(tag SystemTagSet) bool {
}

// Map returns the TagSet with current value from SystemTagSet
func (i *SystemTagSet) Map() TagSet {
func (i SystemTagSet) Map() TagSet {
m := TagSet{}
for _, tag := range SystemTagSetValues() {
if i.Has(tag) {
Expand All @@ -70,6 +71,17 @@ func (i *SystemTagSet) Map() TagSet {
return m
}

// SetString is returns comma separeted list of the string representation of all values in the set
Copy link
Contributor

@cuonglm cuonglm Oct 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in "is returns".

And also, should this method named List(), for alignment with Map() above? And actually return a slice instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the typo and did some .. performance optimizations (not actually needed I am sure).

I don't think it's worth it to add a generic way to get a list of the set values from the SystemTags ... this is literally the only place where it's used ...

func (i SystemTagSet) SetString() string {
m := i.Map()
var keys = make([]string, 0, len(m))
for key := range m {
keys = append(keys, key)
}
sort.Strings(keys)
return strings.Join(keys, ",")
}

// ToSystemTagSet converts list of tags to SystemTagSet
// TODO: emit error instead of discarding invalid values.
func ToSystemTagSet(tags []string) *SystemTagSet {
Expand Down