Influxdb tags as fields

[danron]

This implements:
#569

The code can be optimised so that we don't create a new map each time in func extractFields but I would like some input on how to do that.

To configure this see samples/config.json or you can use the env var INFLUXDB_TAGS_AS_FIELDS. Default value is "iter,vu,url".

[codecov-io]

Codecov Report

Merging #585 into master will decrease coverage by 0.24%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
- Coverage    62.8%   62.55%   -0.25%     
==========================================
  Files          97       97              
  Lines        7374     7406      +32     
==========================================
+ Hits         4631     4633       +2     
- Misses       2489     2521      +32     
+ Partials      254      252       -2

Impacted Files	Coverage Δ
stats/influxdb/config.go	27.94% <0%> (-3.21%)	⬇️
stats/influxdb/collector.go	0% <0%> (ø)	⬆️
cmd/login_influxdb.go	4% <0%> (ø)	⬆️
cmd/collectors.go	0% <0%> (ø)	⬆️
js/modules/k6/metrics/metrics.go	75% <0%> (ø)	⬆️
stats/stats.go	39.18% <0%> (ø)	⬆️
core/engine.go	89.83% <0%> (+1.06%)	⬆️

---

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 16f3fb3...2c0906f. Read the comment docs.

[na--]

I've written some comments, but honestly I'd be fine to merge this as it is. One of the comments is probably a premature optimization, while the other one could be fixed when we update the influxdb configuration to use null-able types like the other configs in k6.
I've locally updated the cloud collector config to use them and I'll create a new issue for the influxdb collector, which will probably happen in conjunction with moving this in lib.Options, as we talked on slack. @luizbafilho, thoughts?

[na--]

The idea with this TODO is that since this pull request, sample.Tags is now comparable, it could be used as a map key for creating a simple cache. If you had something like this outside of the for loop:

cache := map[sample.Tags]struct {
	tags   map[string]string
	values map[string]interface{}
}{}

In the for loop you'd be able to check if certain tags are already in the cache with

if cached, ok := cache[sample.Tags]; ok {
	// Reuse the cached.tags, create a new map with {"value": sample.Value} and copy all elements from cached.values to it
} else {
	// Do what you currently do and then save the resulting tags and values as a new entry in the cache
}

I'm fine with merging this as it is, if you don't want to bother with this probably premature optimization. If it turns out that it's an issue, we'll optimize it in the future.

[danron]

I'm looking at this now so we can hold off with the merge.

[na--]

Hmm I'm not sure that doing this in the UnmarshalText() and UnmarshalJSON() (and actually having a defaultValues() function that changes a receiver's property) is very idiomatic and potentially it could be confusing to Go developers. A much more idiomatic solution would be to create a "constructor" function called NewConfig() that instantiates the config with the default values: https://hassansin.github.io/golang-constructor-like-function-use-cases

This would also solve the issues with the suboptimal tests. The overhead is that elsewhere in the code you'd have to keep in mind that you'd have to use NewConfig() instead of directly instantiating a Config struct or, in those cases:

• https://github.com/loadimpact/k6/blob/master/cmd/login_influxdb.go#L49
• https://github.com/loadimpact/k6/blob/master/cmd/collectors.go#L71

and others like them, something like:

conf := influxdb.NewConfig().Apply(config.Collectors.InfluxDB)

[danron]

I agree, it doesn't feel right as is. I'll take a look at changing this to a NewConfig() function.

[na--]

Here are the other issues that I mentioned with the current influxdb config: #586 and #587

[na--]

There are a few minor issues I've commented in the code, but otherwise it looks good. I think it's a good idea to have the cache in commit(), it will be most useful there and we don't have to worry about it leaking memory.

[na--]

I think that this would be a bug that causes all values["value"] to be the same: https://play.golang.org/p/xikPsAxZWFI
Basically assigning a map with = doesn't do a deep copy of the values and changing the "copy" affects the original :/ One of those unpleasant Go gotchas...
You probably have to create a new map every time in the loop (values := map[string]interface{}{"value": sample.Value}) and then copy all items from cached.values in it with a loop.

[na--]

Hmm actually it's probably a better idea to create a new empty map, copy all cached.values in it and then set values["value"] to sample.Value, just in case someone tries to create a new tag named "value" 😄

[danron]

This actually works but as you said in private chat, I will change it to be safe if InfluxDB changes it's implementation and start caching/batching stuff internally.

[na--]

This should be tags instead of sample.Tags.CloneTags()

[na--]

If you want, you can create a very local type inside the commit() function itself like this:

type cacheItem struct {
	tags   map[string]string
	values map[string]interface{}
}

and then have cache := map[*stats.SampleTags]cacheItem{} and cache[sample.Tags] = cacheItem{tags,values} without having to redeclare the whole struct.

[na--]

See comment above, the whole values generation should probably be outside of the if {} else {} block

[na--]

LGTM, thanks!