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

[elasticsearch] fix pending_tasks mappings #4255

Merged
merged 5 commits into from
Sep 28, 2022

Conversation

klacabane
Copy link
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
Copy link

elasticmachine commented Sep 21, 2022

💚 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
Copy link

elasticmachine commented Sep 21, 2022

🚀 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
Copy link

elasticmachine commented Sep 21, 2022

🌐 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) 💚

Copy link
Contributor

@crespocarlos crespocarlos left a comment

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
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
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
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
Copy link
Contributor Author

klacabane commented Sep 27, 2022

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
Contributor

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

@crespocarlos
Copy link
Contributor

crespocarlos commented Sep 28, 2022

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
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Stack Monitoring] Fix elasticsearch package logo
4 participants