Skip to content

[elasticsearch] fix pending_tasks mappings - #4255

Merged
crespocarlos merged 5 commits into
elastic:mainfrom
klacabane:138865-es-tweaks
Sep 28, 2022
Merged

[elasticsearch] fix pending_tasks mappings#4255
crespocarlos merged 5 commits into
elastic:mainfrom
klacabane:138865-es-tweaks

Conversation

@klacabane

Copy link
Copy Markdown
Contributor

Summary

Closes elastic/kibana#138865

This change contains multiple small fixes:

  • I generated new pending tasks documents and realized the mappings were wrong. Note that the .monitoring-es-mb mappings are also incorrect and should be updated - 789ead0
  • Updated the readme to use template shortcuts. Note that the output readme is extremely noisy and we should improve the readability, for example by wrapping the fields and sample events in a details block
  • Updated the logo
@klacabane klacabane added Integration:elasticsearch Elasticsearch Team:Infra Monitoring UI - DEPRECATED Label for the Infrastructure Monitoring UI team. - DEPRECATED - Use Team:obs-ux-infra_services labels Sep 21, 2022
@klacabane
klacabane requested a review from a team as a code owner September 21, 2022 16:35
@klacabane klacabane self-assigned this Sep 21, 2022
@elasticmachine

elasticmachine commented Sep 21, 2022

Copy link
Copy Markdown

💚 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 preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-22T09:22:03.331+0000

  • Duration: 13 min 0 sec

Test stats 🧪

Test Results
Failed 0
Passed 43
Skipped 0
Total 43

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine

elasticmachine commented Sep 21, 2022

Copy link
Copy Markdown

🚀 Benchmarks report

Package elasticsearch 👍(3) 💚(0) 💔(2)

Expand to view
Data stream Previous EPS New EPS Diff (%) Result
audit 1394.7 1039.5 -355.2 (-25.47%) 💔
gc 8000 6250 -1750 (-21.88%) 💔

To see the full report comment with /test benchmark fullreport

@elasticmachine

elasticmachine commented Sep 21, 2022

Copy link
Copy Markdown

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (5/5) 💚
Files 100.0% (9/9) 💚 2.641
Classes 100.0% (9/9) 💚 2.641
Methods 77.064% (84/109) 👎 -12.811
Lines 91.98% (562/611) 👍 0.649
Conditionals 100.0% (0/0) 💚

@crespocarlos crespocarlos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! I tried, but I didn't manage to create pending_tasks to test the mapping fix. @matschaffer , do you know how?

@klacabane

Copy link
Copy Markdown
Contributor Author

There's probably a better approach but I went the brute force way:

  • create indexes in a while loop, you can have multiple in parallel to increase probability of queuing
  • update metricbeat/agent configuration to collect metrics as often as possible, for example 1s, because tasks won't spent to much time in the queue
@crespocarlos crespocarlos mentioned this pull request Sep 26, 2022
4 tasks
@matschaffer

Copy link
Copy Markdown
Contributor

If I grep kibana source for cluster.pending_task I see number of test mapping references. Are they wrong or does this change only happen after a certain version?

@crespocarlos

Copy link
Copy Markdown
Contributor

Thanks @klacabane. I managed to have some pending tasks here.

@matschaffer, indeed there is a number of cluster.pending_task in the Kibana repo. I've run some tests reverting the changes present here.

Currently, without Kevin's change, pending_task is an object within cluster object

GET metrics-elasticsearch.stack_monitoring.pending_tasks-default/_mapping

"elasticsearch": {
  "properties": {
    "cluster": {
      "properties": {
        "id": {
          "type": "keyword",
          "ignore_above": 1024
        },
        "name": {
          "type": "keyword",
          "ignore_above": 1024
        },
        "pending_task": { <--- current mapping
          "properties": {
            "insert_order": {
              "type": "long"
            },
            "priority": {
              "type": "keyword",
              "ignore_above": 1024
            },
            "source": {
              "type": "keyword",
              "ignore_above": 1024
            },
            "time_in_queue": {
              "properties": {
                "ms": {
                  "type": "long"
                }
              }
            }
          }
        },
        "state": {
          "properties": {
            "id": {
              "type": "keyword",
              "ignore_above": 1024
            }
          }
        }
      }
    },
    "node": {
      "properties": {
        "id": {
          "type": "keyword",
          "ignore_above": 1024
        },
        "master": {
          "type": "boolean"
        },
        "mlockall": {
          "type": "boolean"
        },
        "name": {
          "type": "keyword",
          "ignore_above": 1024
        }
      }
    }
  }
}

And this is what is actually in the doc. Looks like the mapping is wrong.

GET metrics-elasticsearch.stack_monitoring.pending_tasks-default/_search

"elasticsearch": {
  "cluster": {
    "name": "elasticsearch",
    "id": "CvwadJe-QPyh9S7YKTNLAQ"
  },
  "pending_tasks": {
    "time_in_queue.ms": 150,
    "source": "create-index [test_e+cu4r+1], cause [api]",
    "priority": "URGENT",
    "insert_order": 1336
  }
}
@klacabane

klacabane commented Sep 27, 2022

Copy link
Copy Markdown
Contributor Author

Looks like the mappings are correct for version up to 6.4 (see https://github.com/elastic/beats/blob/6.4/metricbeat/module/elasticsearch/pending_tasks/pending_tasks.go#L90), starting 6.5 we write to elasticsearch.pending_tasks. What's curious is that (the document was updated in place since timestamp remains the same) we see changes to the reference document made after that, for example in 7.0, but with the old schema

@matschaffer

Copy link
Copy Markdown
Contributor

Gotcha. Let's at least get an issue open to fix the other mappings (internal/mb, maybe fixture data?).

@crespocarlos

crespocarlos commented Sep 28, 2022

Copy link
Copy Markdown
Contributor

I've opened this one for mb: elastic/beats#33211 and es mapping has already been updated elastic/elasticsearch#90203. Fixture data would be nice too.
The problem is that we end up running integration tests against old mappings, since those are not updated automatically. It's very likely that there are a number of other outdated mappings there.

@crespocarlos
crespocarlos merged commit 88212ef into elastic:main Sep 28, 2022
orestisfl pushed a commit to orestisfl/integrations that referenced this pull request May 15, 2026
* update logo

* update readme

* fix pending_tasks mappings

* generate readme

* move elastic version to .env file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Integration:elasticsearch Elasticsearch Team:Infra Monitoring UI - DEPRECATED Label for the Infrastructure Monitoring UI team. - DEPRECATED - Use Team:obs-ux-infra_services

4 participants