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

[receiver/elasticsearch] Request to enable more metrics by default #34396

Closed
crobert-1 opened this issue Aug 2, 2024 · 2 comments · Fixed by #34397
Closed

[receiver/elasticsearch] Request to enable more metrics by default #34396

crobert-1 opened this issue Aug 2, 2024 · 2 comments · Fixed by #34397

Comments

@crobert-1
Copy link
Member

Component(s)

receiver/elasticsearch

Describe the issue you're reporting

This is a request to enable a few existing metrics by default, that are currently disabled by default.

Requested metrics

Here's the list of metrics:

elasticsearch.index.documents: The number of documents for an index
elasticsearch.index.operations.merge.current: The number of currently active segment merges
elasticsearch.index.segments.count: Number of segments of an index.

Justification

These metrics are all important in understanding the current status of an Elasticsearch index.

The count of documents in an index is helpful in understanding the size and contents of an index.

The current merge count is also important to see the state of the cluster due to the potential performance impacts of the ongoing index merges.

The segment count of an index is also important to understand the current state of an index, and potential performance impacts of how many segments are in it (more segments can result in longer search times).

Counter-point to Justification

This component is marked as beta stability, and enabling metrics by default is considered a breaking change. Breaking changes are allowed, but should be minimized, according to the stability definition referenced.

Discussion is welcome, I can open a PR if this is acceptable to code owners.

@crobert-1 crobert-1 added the needs triage New item requiring triage label Aug 2, 2024
Copy link
Contributor

github-actions bot commented Aug 2, 2024

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@crobert-1
Copy link
Member Author

Removing needs triage as a code owner has responded with a 👍. I'll post a PR.

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Aug 2, 2024
djaglowski pushed a commit that referenced this issue Aug 5, 2024
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
This enables the following metrics by default:

```
elasticsearch.index.documents
elasticsearch.index.operations.merge.current
elasticsearch.index.segments.count
```

**Link to tracking Issue:** <Issue number if applicable>
Resolves #34396

**Testing:** <Describe what testing was performed and which tests were
added.>
Tests have been updated to account for these metrics being enabled by
default. One thing to note, some tests were manually enabling these
metrics. This is no longer necessary, but I've left as-is since it
doesn't do any harm, and can be helpful to ensure tests act the way we
want. I can update though if reviewers prefer.

Testing side note: For some reason, when running integration tests
locally, I had to add a `time.Sleep` call before scraping when the
docker container was being spun up. Some metrics and resource attributes
weren't populated fully without the delay, and caused a mismatch with
the GitHub CI's results that are generated without the sleep call.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant