Add safemapstr.Put
#6434
Add safemapstr.Put
#6434
Conversation
exekias
force-pushed
the
mapstr-dedot
branch
from
9aa744e to
e0c3c0f
Compare
|
@ruflin I came up with this while working on "dedoting", what do you think? |
|
TBH I'm kind of -1 on this. First of all, common.MapStr is already a 'monster'. Please let's not add more methods on top of it. There is some magic going on, which might not be always clear when and how it is triggered. The docker swarm case of having these kind of labels is quite bad practice. When trying to add some hierarchy into labels using dots, the labels open up some kind of namespacing. Therefore a label string must not be a prefix of another label. This should be the case in general, not just in the context of beats. Instead of adding the DeDot functionality to common.MapStr, how about having autodiscovery (or some other module) detect violations in label use and fix the label naming there. We could also introduce some label-rewriting rules (with default rules). Also consider opening an issue against docker-swarm to use sane labels (what's the difference between |
|
Thanks for the feedback, sounds good to me! I'll create a helper function to build the safe MapStr somewhere outside MapStr code 👍 |
exekias
force-pushed
the
mapstr-dedot
branch
from
e0c3c0f to
c401d92
Compare
exekias
force-pushed
the
mapstr-dedot
branch
from
c401d92 to
9f37148
Compare
|
I moved the code to a new |
This method implements a way to put dotted keys into a MapStr while
ensuring they don't override each other. For example:
```
a := MapStr{}
a.Put("com.docker.swarm.task", "x")
a.Put("com.docker.swarm.task.id", 1)
a.Put("com.docker.swarm.task.name", "foobar")
```
Will result in `{"com":{"docker":{"swarm":{"task":"x"}}}}`
DeDotPut detects this scenario and renames the common base key, by appending
`.value`:
```
a := MapStr{}
safemapstr.Put(a, "com.docker.swarm.task", "x")
safemapstr.Put(a, "com.docker.swarm.task.id", 1)
safemapstr.Put(a, "com.docker.swarm.task.name", "foobar")
```
Will result in `{"com":{"docker":{"swarm":{"task":{"id":1,"name":"foobar","value":"x"}}}}}`
The plan is to use this method to convert all unstructured external data, like labels, annotations and so
Related to elastic#6421
exekias
force-pushed
the
mapstr-dedot
branch
from
9f37148 to
665d935
Compare
| // | ||
| // Put detects this scenario and renames the common base key, by appending | ||
| // `.value` | ||
| func Put(data common.MapStr, key string, value interface{}) error { |
There was a problem hiding this comment.
how about introducing a 'defaultKey' to be used instead of hard-coded 'value' ?
There was a problem hiding this comment.
do you mean moving it to a const or passing it as a parameter to Put?
There was a problem hiding this comment.
parameter. I wonder if in some cases we can declare/define a better name then 'value'.
| // Put detects this scenario and renames the common base key, by appending | ||
| // `.value` | ||
| func Put(data common.MapStr, key string, value interface{}) error { | ||
| keyParts := strings.SplitN(key, ".", 2) |
There was a problem hiding this comment.
I see it's a copy'n paste of MapStr.Put.
Using strings.Index, sub-slices and loop might make the routine even faster.
|
jenkins, retest it please |
| if len(keyParts) == 1 { | ||
| oldValue, exists := data[key] | ||
| if exists { | ||
| switch oldValue.(type) { |
There was a problem hiding this comment.
I think you should use tryToMapStr here to be consistent with the handling of common.MapStr and map[string]interface{}.
There was a problem hiding this comment.
Nice catch, pushing a change to make use of it 😇
Add
safemapstr.Put:This method implements a way to put dotted keys into a MapStr while
ensuring they don't override each other when they are not really prepared to be stored in a tree. For example:
Will result in
{"com":{"docker":{"swarm":{"task":"x"}}}}Put detects this scenario and renames the common base key, by appending
.value:Will result in
{"com":{"docker":{"swarm":{"task":{"id":1,"name":"foobar","value":"x"}}}}}The plan is to use this method to convert all unstructured external data, like labels, annotations and so
Related to #6421