-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Unified Observability] update network fields #134471
Conversation
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
Pinging @elastic/unified-observability (Team:Unified observability) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍🏼
}, | ||
], | ||
split_mode: 'terms', | ||
terms_field: 'system.network.name', | ||
split_mode: 'everything', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x-pack/plugins/infra/common/inventory_models/host/metrics/tsvb/host_network_traffic.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool stuff. Just added a couple of comments
I think we need to update the files in x-pack/test/functional/es_archives/infra/8.0.0/hosts_only
so that the tests can reflect these changes
x-pack/plugins/infra/common/inventory_models/host/metrics/snapshot/rx.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/infra/common/inventory_models/host/metrics/snapshot/tx.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two spots I noted that I think we should revert the tests. The first one is the Metrics Explorer test that confirms the "rate" aggregation is using the correct backing aggregations for Elasticsearch. Both tests used the field name system.network.in.bytes
BUT they are not necessarily tied to that metric. We should change the field from system.network.in.bytes
to counter
for the tests so it's more ambiguous.
...lugins/infra/server/routes/metrics_explorer/lib/convert_metric_to_metrics_api_metric.test.ts
Show resolved
Hide resolved
x-pack/plugins/infra/server/lib/metrics/lib/create_metrics_aggregations.test.ts
Show resolved
Hide resolved
7091fce
to
6c755ed
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💛 Build succeeded, but was flakyFailed CI StepsTest Failures
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for reverting the tests.
Resolves #131152
Swaps out usage of non ECS fields
system.network.in.bytes
andsystem.network.out.bytes
(counters) for ECS fieldshost.network.ingress.bytes
andhost.network.egress.bytes
which are gauges. These new ECS fields were added in metricbeat 7.14 so users who are on Kibana 8.4+ but on a version of metricbeat prior to 7.14 will have no data until they upgrade metricbeat to at least 7.14.The new ECS fields according to the docs:
The number of bytes received/sent out (gauge) on all network interfaces by the host since the last metric collection.
The main calculation we now need to do is calculate bytes per second. Since this depends on the user's
metricset.period
based on the docs, we use(avg) host.network.ingress.bytes / (metricset.period / 1000)
In order to make sure the changes made matched the previous field's calculations I used a TSVB chart to compare:
The query aggs look as follows now:
I also had to fix an existing bug where we only got the default of 10 interfaces. I found this issue while using my mac as it has a lot more interfaces and I wasn't getting the correct data in any of these places where we report rx/tx. Notice in the tsvb chart I changed the "Top" to 100 under system.network.in.bytes vis in order for the chart to match. We won't need to worry about this when using the new ECS fields because it returns the value of "all network interfaces".
Docs will be updated in another PR.
The following places are affected:
Metrics: "Outbound traffic" and "Inbound traffic" the overlay and Network vis in popover:
Metrics: Inventory panels and vis:
Observability Overview: RX/TX
Inventory Threshold Rule for Inbound/Outbound traffic