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][Ingest Pipeline] Add elasticsearch.node.name as TSDS dimension #11527

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BenB196
Copy link
Contributor

@BenB196 BenB196 commented Oct 26, 2024

Bug

Proposed commit message

Adds elasticsearch.node.name as a TSDS dimension to prevent document collisions.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.
  • I have verified that any added dashboard complies with Kibana's Dashboard good practices

Author's Checklist

  • [ ]

How to test this PR locally

Install assets for the Elasticsearch integration and validate that the metrics-elasticsearch.ingest_pipeline@package component template properly sets elasticsearch.node.name as "time_series_dimension": true

Related issues

Screenshots

image

@BenB196 BenB196 requested a review from a team as a code owner October 26, 2024 12:37
consulthys
consulthys previously approved these changes Oct 26, 2024
Copy link
Contributor

@consulthys consulthys left a comment

Choose a reason for hiding this comment

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

Well spotted!
Thanks for the contribution

@consulthys
Copy link
Contributor

consulthys commented Oct 26, 2024

Interestingly for the node_stats metric set, the elasticsearch.node.name was added as a dimension

Can you check this commit to see what would make the most sense regarding other metric sets?

@consulthys consulthys requested review from consulthys and a team and removed request for consulthys October 26, 2024 13:08
@consulthys consulthys dismissed their stale review October 26, 2024 13:09

Asking for more checks

@BenB196
Copy link
Contributor Author

BenB196 commented Oct 26, 2024

Interestingly for the node_stats metric set, the elasticsearch.node.name was added as a dimension

Can you check this commit to see what would make the most sense regarding other metric sets?

Taking a look at the example provided, I think either would work, service.address being a dimension should prevent collisions between clusters if 2 clusters have the same elasticsearch.node.name value and are collected by the same centralized agent.

I've pushed a commit to use node.name here as with the other one.

@BenB196 BenB196 changed the title [Elasticsearch][Ingest Pipeline] Add elasticsearch.node.id as TSDS dimension [Elasticsearch][Ingest Pipeline] Add elasticsearch.node.name as TSDS dimension Oct 26, 2024
@consulthys
Copy link
Contributor

Agreed, because elasticsearch.node.id is also not guaranteed to be unique across clusters, so either would be fine, but since other metric sets use elasticsearch.node.name it'd make sense to be consistent in this one also.

Copy link
Contributor

@consulthys consulthys left a comment

Choose a reason for hiding this comment

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

LGTM

@consulthys consulthys added Integration:elasticsearch Elasticsearch Feature:Stack Monitoring Stack Monitoring Feature Team:Stack Monitoring Stack Monitoring team [elastic/stack-monitoring] labels Oct 26, 2024
@andrewkroh andrewkroh added the bugfix Pull request that fixes a bug issue label Oct 28, 2024
@consulthys
Copy link
Contributor

/test

@elastic-vault-github-plugin-prod

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@elasticmachine
Copy link

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a bug issue Feature:Stack Monitoring Stack Monitoring Feature Integration:elasticsearch Elasticsearch Team:Stack Monitoring Stack Monitoring team [elastic/stack-monitoring]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Elasticsearch Ingest Pipeline]: Missing TSDS Dimension on Elasticsearch Node Id field
4 participants