Skip to content
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

[Metricbeat] Remove elasticsearch.index.created from the SM code #25113

Merged
merged 4 commits into from
Apr 20, 2021

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Apr 15, 2021

What does this PR do?

SM index metricset does an API HTTP call to elasticsearch to retrieve index metadata information. This information is required to populate field elasticsearch.index.created or (index_stats.created if xpack.enabled: true when configuring the module).

The code removes the API call to /_cluster/state/metadata.

Why is it important?

The HTTP call to /_cluster/state/metadata can be VERY expensive. The reason for this is that it returns the entire mapping of all indices on a cluster. If a user has many indices and / or each index has many fields, this could lead to very heavy responses from elasticsearchr (20 Mb in big clusters measured with curl).

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • This is a breaking change, because users could be using field elasticsearch.index.created or index_stats.created and they will be removed.
  • Stack Monitoring UI seems to keep working without any issues. I don't see this field populated anywhere.

@sayden sayden self-assigned this Apr 15, 2021
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 15, 2021
@sayden sayden added Feature:Stack Monitoring Team:Services (Deprecated) Label for the former Integrations-Services team labels Apr 15, 2021
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Apr 15, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 15, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Pull request #25113 updated

  • Start Time: 2021-04-20T10:04:23.844+0000

  • Duration: 76 min 5 sec

  • Commit: 0009968

Test stats 🧪

Test Results
Failed 0
Passed 8339
Skipped 2270
Total 10609

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 8339
Skipped 2270
Total 10609

@ycombinator
Copy link
Contributor

Stack Monitoring UI seems to keep working without any issues. I don't see this field populated anywhere.

@sayden This would be my main concern with removing this field. Can we get someone from @elastic/stack-monitoring-ui team to confirm that removing this field will have no impact on the UI? Once we have that, I'm good with removing it to reduce the expensive API call.

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm there are no references to any field named "created" in the Stack Monitoring UI at this moment.

For later reference, here is a search for "created" in x-pack/plugins/monitoring (excluding snapshot tests and SVG files):

Screen Shot 2021-04-19 at 12 12 47 PM

``` 15 results - 10 files

x-pack/plugins/monitoring/public/plugin.ts:
32 import { createLegacyAlertTypes } from './alerts/legacy_alert';
33: import { createDiskUsageAlertType } from './alerts/disk_usage_alert';
34 import { createThreadPoolRejectionsAlertType } from './alerts/thread_pool_rejections_alert';

172 alertTypeRegistry.register(createCpuUsageAlertType());
173: alertTypeRegistry.register(createDiskUsageAlertType());
174 alertTypeRegistry.register(createMemoryUsageAlertType());

x-pack/plugins/monitoring/public/alerts/disk_usage_alert/index.tsx:
19
20: export function createDiskUsageAlertType(): AlertTypeModel {
21 return {

x-pack/plugins/monitoring/public/alerts/lib/alerts_toast.tsx:
81 id="xpack.monitoring.healthCheck.disabledWatches.title"
82: defaultMessage="New alerts created"
83 />

x-pack/plugins/monitoring/public/alerts/lib/get_alert_panels_by_category.test.tsx:
48 params: {},
49: createdBy: null,
50 updatedBy: null,
51: createdAt: new Date('2020-12-08'),
52 updatedAt: new Date('2020-12-08'),

x-pack/plugins/monitoring/public/alerts/lib/get_alert_panels_by_node.test.tsx:
46 params: {},
47: createdBy: null,
48 updatedBy: null,
49: createdAt: new Date('2020-12-08'),
50 updatedAt: new Date('2020-12-08'),

x-pack/plugins/monitoring/server/lib/mb_safe_query.ts:
11 * queries/aggs continue to work but we need to handle the reality that these aliases will not
12: * exist for older metricbeat-* indices, created before the aliases existed.
13 *

x-pack/plugins/monitoring/server/lib/details/get_series.js:
245 console.log(
246: `metric.debug field=${metric.field} bucketsCreated: ${countBuckets(
247 get(response, 'aggregations.check')

x-pack/plugins/monitoring/server/lib/elasticsearch/convert_metric_names.js:
17 *
18: * Historically, the get_nodes function created an aggregation with multiple sub date_histogram
19 * aggregations for each metric aggregation. From a top down view, the entire aggregations look liked:

x-pack/plugins/monitoring/server/lib/metrics/beats/metrics.js:
72 description: i18n.translate('xpack.monitoring.metrics.beats.eventsRate.totalDescription', {
73: defaultMessage: 'All events newly created in the publishing pipeline',
74 }),

x-pack/plugins/monitoring/server/routes/api/v1/alerts/enable.ts:
79
80: let createdAlerts: Array<SanitizedAlert> = [];
81 const disabledWatcherClusterAlerts = await disableWatcherClusterAlerts(

86 if (disabledWatcherClusterAlerts) {
87: createdAlerts = await Promise.all(
88 alerts.map((alert) => alert.createIfDoesNotExist(alertsClient, actionsClient, actions))

91
92: return response.ok({ body: { createdAlerts, disabledWatcherClusterAlerts } });
93 } catch (err) {


</details>

if err != nil {
return errors.Wrap(err, "failed to get index creation time")
}
idx.Created = created
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the Created field from the struct as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point

@ycombinator
Copy link
Contributor

Thanks for checking and confirming, @jasonrhodes.

@ycombinator
Copy link
Contributor

ycombinator commented Apr 19, 2021

The code removes the API call to /_cluster/state/metadata.

@sayden Reviewing the code changes in this PR, it looks like we are not actually removing the API call to ES. This PR is simply removing the code that parses the cluster state API response to extract the index metadata field from it.

@sayden
Copy link
Contributor Author

sayden commented Apr 19, 2021

The code removes the API call to /_cluster/state/metadata.

@sayden Reviewing the code changes in this PR, it looks like we are not actually removing the API call to ES. This PR is simply removing the code that parses the cluster state API response to extract the index metadata field from it.

Yes, sorry for that. I cherry-picked the commit from a different branch and I didn't realize that I split the change in two commits

@sayden
Copy link
Contributor Author

sayden commented Apr 19, 2021

Ok, I actually removed the call now 😅 (actually, the request into a different call), deleted the field in struct and added a changelog entry into breaking changes.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@sayden
Copy link
Contributor Author

sayden commented Apr 19, 2021

Thank you for your help too, @jasonrhodes

@sayden sayden marked this pull request as ready for review April 20, 2021 11:32
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-services (Team:Services)

@sayden sayden merged commit a951d19 into master Apr 20, 2021
@sayden sayden added the v7.13.0 label Apr 20, 2021
sayden added a commit to sayden/beats that referenced this pull request Apr 20, 2021
sayden added a commit that referenced this pull request Apr 20, 2021
v1v added a commit to v1v/beats that referenced this pull request Apr 21, 2021
…-github-pr-comment-template

* upstream/master:
  Check native environment before starting (elastic#25186)
  Change event.code and winlog.event_id type (elastic#25176)
  [Ingest Manager] Proxy processes/elastic-agent to stats (elastic#25193)
  Update mergify backporting to 7.x and 7.13 (elastic#25196)
  [Heartbeat]: ensure synthetics version co* [Heartbeat]: ensure synthetics version compatability for suites  * address review and fix notice  * fix lowercase struct  * fix version conflict and rebase  * update go.* stuff to master  * fix notice.txt  * move validate inside sourcempatability for suites (elastic#24777)
  [Filebeat] Ensure Kibana audit `event.category` and `event.type` are still processed as strings. (elastic#25101)
  Update replace.asciidoc (elastic#25055)
  Fix nil panic when overwriting metadata (elastic#24741)
  [Filebeat] Add Malware Bazaar to Threat Intel Module (elastic#24570)
  Fix k8s svc selectors mapping (elastic#25169)
  [Ingest Manager] Make agent retry values for bootstraping configurable (elastic#25163)
  [Metricbeat] Remove elasticsearc.index.created from the SM code (elastic#25113)
v1v added a commit to v1v/beats that referenced this pull request Apr 22, 2021
…ng-versions-stack

* upstream/master: (28 commits)
  Add support for parsers in filestream input (elastic#24763)
  Skip flaky test TestFilestreamTruncate (elastic#25218)
  backport: Add 7.13 branch (elastic#25189)
  Update decode_json_fields.asciidoc (elastic#25056)
  [Elastic Agent] Fix status and inspect command to work inside running container (elastic#25204)
  Check native environment before starting (elastic#25186)
  Change event.code and winlog.event_id type (elastic#25176)
  [Ingest Manager] Proxy processes/elastic-agent to stats (elastic#25193)
  Update mergify backporting to 7.x and 7.13 (elastic#25196)
  [Heartbeat]: ensure synthetics version co* [Heartbeat]: ensure synthetics version compatability for suites  * address review and fix notice  * fix lowercase struct  * fix version conflict and rebase  * update go.* stuff to master  * fix notice.txt  * move validate inside sourcempatability for suites (elastic#24777)
  [Filebeat] Ensure Kibana audit `event.category` and `event.type` are still processed as strings. (elastic#25101)
  Update replace.asciidoc (elastic#25055)
  Fix nil panic when overwriting metadata (elastic#24741)
  [Filebeat] Add Malware Bazaar to Threat Intel Module (elastic#24570)
  Fix k8s svc selectors mapping (elastic#25169)
  [Ingest Manager] Make agent retry values for bootstraping configurable (elastic#25163)
  [Metricbeat] Remove elasticsearc.index.created from the SM code (elastic#25113)
  [Ingest Manager] Keep http and logging config during enroll (elastic#25132)
  Refactor kubernetes autodiscover to avoid skipping short-living pods (elastic#24742)
  [libbeat] New decode xml wineventlog processor (elastic#25115)
  ...
@Leaf-Lin Leaf-Lin changed the title [Metricbeat] Remove elasticsearc.index.created from the SM code [Metricbeat] Remove elasticsearch.index.created from the SM code Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants