Adopt the new schema approach for the MongoDb module #2040
Conversation
tsg
force-pushed
the
mongodb_refactor_take2
branch
from
7826c78 to
c6d3b29
Compare
| @@ -0,0 +1,97 @@ | |||
| package status | |||
There was a problem hiding this comment.
Should this code be moved into the helper?
There was a problem hiding this comment.
Yeah, I'm trying to find a way to organize these in a way that makes sense. We basically have two sets of convertors. One set that work on map[string]string (one level deep) and one that work on map[string]interface{} (nested). I'm thinking to have something like:
helper/
schema.go
mapstrstr/
...
mapstriface/
...
The names are not great, I know. We could also either move the whole thing one level down under helper/schema/ or replace helper with schema as there are no other helpers.
There was a problem hiding this comment.
+1 on moving it to schema as a top level package as this is so critical to building metricsets. Also then we can use s.* instead of h.* which somehow feels more logical. Or would you use the full package name every time?
| "workers": common.MapStr{ | ||
| "busy": h.Int("BusyWorkers"), | ||
| "idle": h.Int("IdleWorkers"), | ||
| schema_ = schema.Schema{ |
There was a problem hiding this comment.
Little bit odd naming convention, especially as it will also be used by others. Perhaps going with s and using c for the s. C would stand for conversion. BTW: if we use . for the include mapstrstr. Never tried it but that should make it part of the same namespace. So we could use it without prefix.
There was a problem hiding this comment.
So we'll have s.Schema and c.Int, right? Yeah, that could also work.
I'd prefer not going to dot imports, those are usually more messy and upset linters.
There was a problem hiding this comment.
yeah, please no dot imports if possible. I'd also put all functions/types used to define a schema into one package (or at least re-export).
There was a problem hiding this comment.
All in one package wouldn't be possible without significant repetition between mapstrstr and mapstriface. What do you mean by re-export. I tried something like type Schema schema.Schema but that doesn't really work because the resulting type is not compatible.
|
LGTM. So minor naming comments. Huge step forward. That will make creating metricsets even easier. |
|
jenkins, retest it |
tsg
force-pushed
the
mongodb_refactor_take2
branch
from
f68dda5 to
2ee410b
Compare
| "testErrorBool": "yes", // invalid bool | ||
| } | ||
|
|
||
| schema_ := schema.Schema{ |
There was a problem hiding this comment.
Still schema_ but not critical as it is part of the tests.
This is applying the ideas from elastic#2034 to the MongoDB module, simplifying the code significantly. Besides simplifying the logic, this fixes a bug: in the previous version if an error happened on a key that was named differently from the source, the key was not removed.
* Generalize the schema to work on map[string]interface{}. This drops a
bit on the type safety but makes it applicable in more cases. In
particular, it makes it possible to reuse the defined types and code
in the MongoDB module.
* Added a Mapper interface for Conv and map[string]Conv. This adds type
safety to the schema.
* Added unit tests
tsg
force-pushed
the
mongodb_refactor_take2
branch
from
2ee410b to
fc76d70
Compare
|
History cleaned up, let's wait for green. I'm preparing another PR for adding some docs to the dev guide. |
This is applying the ideas from #2034 to the MongoDB module, simplifying the
code significantly. Unfortunately, the same conversions functions cannot be applied
as is to the map[string]interface{} that we get from the mongodb driver, so I had to
reimplement them.
Besides simplifying the logic, this fixes a bug: in the previous version if an error happened on a key that was named differently from the source, the key was not removed.
This PR also refactors and generalizes the conversions schema and organizes them in two sub-packages:
mapstrstrandmapstriface.