Fix and enable apache test on SLES and CentOS 7 #1166
Conversation
martijnvans
changed the title
…sles' into martijnvans-apache-sles
…sles' into martijnvans-apache-sles
| @@ -49,6 +49,9 @@ type ExpectedMetric struct { | |||
| Representative bool `yaml:"representative,omitempty" validate:"excluded_with=Optional,excluded_with=Platform"` | |||
| // Exclusive metric to a particular kind of platform. | |||
| Platform string `yaml:"platform,omitempty" validate:"excluded_with=Representative,omitempty,oneof=linux windows"` | |||
| // A list of platforms that this metric is not available on. | |||
| // Examples: centos-7,debian-10. Not valid are linux,windows. | |||
| UnavailableOn []string `yaml:"unavailable_on,omitempty" validate:"excluded_with=Representative"` | |||
There was a problem hiding this comment.
I wonder if this information also needs to be propagated into the generated documentation… @sophieyfang WDYT?
There was a problem hiding this comment.
The metric side "unavailable on" is at the metric type level. The logging side seems at the field level. I feel like the field level shall be marked as "optional" instead of being "unavailable on". It might make more sense to only use "Unavailable on" when the metric or log doesn't exist for such a platform.
The metric side public doc has a consistent style look. See Ops Agent Metrics public doc, since that doc has a consistent style across all metrics public docs (especially that table look), I wonder where we can expose this info: That certain metric or log isn't available for certain platform. @mtabasko for thoughts.
Personally I think it's not urgent to reflect it to the public doc and definitely not reason to block this PR to be submitted by that.
There was a problem hiding this comment.
sophie and i discussed this a bit, and we agreed that since we don't surface "optional" for log fields or metrics in the docs, we don't need to surface "unavailable_on" either.
If we decided to surface that information later, we could do it by saying something like "this metric/log field may be unavailable on some platforms" if optional or unavailable_on are specified.
regarding sophie's suggestion to use "optional' instead of 'unavailable_on" for log fields, we haven't decided yet. I'm OK with that approach but I think unavailable_on is going to produce slightly better test coverage. The difference is probably minimal though.
martijnvans
requested review from
a team and
rafaelwestphal
and removed request for
a team
| } | ||
| return result | ||
| } | ||
|
|
||
| func runLoggingTestCases(ctx context.Context, logger *logging.DirectoryLogger, vm *gce.VM, logs []*metadata.ExpectedLog) error { |
There was a problem hiding this comment.
Since this is related, can we remove the hack related to b/239240173 on line 602 (bug is marked as fixed)?
If we can not, change it to this new format.
There was a problem hiding this comment.
I updated the bug number. I think the bug is still not fixed. I'm not sure what you mean about changing it to the new format; the feature in this PR cannot be used for aerospike + SLES because this PR only includes a feature for missing log FIELDS, not logs that are entirely missing. It wouldn't be hard to add that feature though, do you want me to do that in this PR or a separate one?
There was a problem hiding this comment.
ah ok, got it. Thanks for updating it.
Description
This PR adds two new features in service of removing the last few
platforms_to_skipfor apache:metadata.yaml.This is better than just marking the logs/metrics as optional, because they can be important on some platforms (so we do want coverage).
Related issue
#1173
How has this been tested?
automated tests only.
Checklist: