expected_metrics follow-up #521
Conversation
This step is to normalize all the metadata files so that they have consistent ordering and formatting, resulting in smaller diffs in the future whenever the generation script is run again.
|
I don't feel strongly about including the regenerated metadata.yaml files and I'd be OK with reverting it if others feel they should be left unchanged. My rationale was to minimize diffs when the script is run in the future, but the downside is that it'll cause merge conflicts for anyone actively working on them, and I'm not sure how much use the generator script gets anyway. |
* Can't use `validate.Struct` on slices
* Can use `validate.Var(metrics, "required,dive")` but I want more
useful error messages so I'm just calling `validate.Struct` on each
element instead
* `validator.Validate` needs to be used as a singleton
* Remove manual check for representative+optional
* `excluded_with` works nicely for this, and it even takes into
account when values are default instead of missing, so
`representative: true, optional: false` is allowed as it
should be
| @@ -1,4 +1,4 @@ | |||
| public_url: "https://cloud.google.com/stackdriver/docs/solutions/agents/ops-agent/third-party/apache" | |||
| public_url: https://cloud.google.com/stackdriver/docs/solutions/agents/ops-agent/third-party/apache | |||
| short_name: apache | |||
There was a problem hiding this comment.
There are quite some changes in the per integration metadata.yaml files. Some are reordering. May i ask what's the reason to reorder them?🤔
There was a problem hiding this comment.
This was just the result of running the generate script on the existing yaml files -- the yaml marshaller doesn't preserve ordering or token formatting by default so the result is that things got out of order when reading and rewriting the file.
I would be fine with reverting all of these yaml changes in this PR since it is indeed quite a lot of changes. The caveat is that the next time someone runs the generate script, it'll change the ordering anyway.
There was a problem hiding this comment.
The thing with the current code is that it changes too many files unnecessarily. It will need reviewer carefully skim over one by one to see that nothing gets wrong.
Maybe we shall fix the code in generate.go thus it shall preserve the ordering. This shall save some chaos in the future too(for those runs the generate.go, i suppose the current code will always reorder them): https://stackoverflow.com/questions/33639269/preserving-order-of-yaml-maps-using-go
There was a problem hiding this comment.
I think I tried MapSlice and ran into some walls with being able to continue using structured types with it. I also tried going deeper using v3 of the package which lets you access the underlying token stream but it still formats things slightly differently (arrays are indented in v3 but not in v2 and there's not enough control over it).
So for now I think a better solution is to just pull out all of these yaml changes to reduce the diff size.
There was a problem hiding this comment.
SGTM!
Didn't know in golang it needs this much work and still can't get it to work.. in python, there is oyaml that will just do the trick.
Another way i can think of is we can make generate.go only print out expected_metrics in the terminal, and letting the code submitter manually modify the metadata.yaml with the provided info showed in the terminal
There was a problem hiding this comment.
True, that would make the code much simpler too... I'll give that a try, good suggestion!
| @@ -1,4 +1,4 @@ | |||
| public_url: "https://cloud.google.com/stackdriver/docs/solutions/agents/ops-agent/third-party/apache" | |||
| public_url: https://cloud.google.com/stackdriver/docs/solutions/agents/ops-agent/third-party/apache | |||
| short_name: apache | |||
There was a problem hiding this comment.
The thing with the current code is that it changes too many files unnecessarily. It will need reviewer carefully skim over one by one to see that nothing gets wrong.
Maybe we shall fix the code in generate.go thus it shall preserve the ordering. This shall save some chaos in the future too(for those runs the generate.go, i suppose the current code will always reorder them): https://stackoverflow.com/questions/33639269/preserving-order-of-yaml-maps-using-go
This reverts commit 378237e.
integration_test/common/common.go
Outdated
| @@ -29,55 +29,71 @@ type ExpectedMetric struct { | |||
| // The metric type, for example workload.googleapis.com/apache.current_connections. | |||
| Type string `yaml:"type"` | |||
There was a problem hiding this comment.
Shall we make this required and also below fields.
There was a problem hiding this comment.
Good catch, will do.
This is a follow-up PR to address optional feedback from the last PR (#456), as well as to update the generator script to use the new metadata.yaml format.