-
Notifications
You must be signed in to change notification settings - Fork 456
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
[prometheus] Eliminate labels fingerprint #9785
[prometheus] Eliminate labels fingerprint #9785
Conversation
…etheus_fingerprint
ObservationData collection with TSDB enabled: Found 1 doc in the discover Data collection without TSDB enabled: Found 64 docs in the discover Both setup having the same instance with same configurations. In the TSDB disabled setup, all the 64 docs are having different prometheus.labels.* field with same timestamp. It seems like the valid documents are dropping by enabling TSDB! |
Thanks @harnish-elastic for validating. For changes involving TSDB, especially when the dimension fields are removed, please run the TSDB test toolkit and include it in the PR. It helps to detect if any documents are dropped. |
@agithomas, I have also run the TSDB test toolkit, the observation is as below, Run test toolkit without TSDB enabled in Kibana 8.13.0, OutputValues being used: elasticsearch_host = https://localhost:9200 elasticsearch_ca_path = /root/.elastic-package/profiles/default/certs/elasticsearch/ca-cert.pem elasticsearch_user = elastic elasticsearch_pwd = changeme data_stream = metrics-prometheus.collector-default docs_index = 0 settings_mappings_index = Last index of the data stream max_docs = All documents get_overlapping_files = True directory_overlapping_files = overwritten-docs-metrics-prometheus.collector-default display_docs = 10 copy_docs_per_dimension = 2You're testing with version 8.13.0. Testing data stream metrics-prometheus.collector-default. Routing path is empty. Program will end. |
When using stack version {
"prometheus.labels.*": {
"path_match": "prometheus.labels.*",
"match_mapping_type": "string",
"mapping": {
"type": "keyword"
}
}
}, Dynamic mappings for {
"prometheus.labels.*": {
"path_match": "prometheus.labels.*",
"match_mapping_type": "string",
"mapping": {
"time_series_dimension": true,
"type": "keyword"
}
}
}, TSDB test:
Each label is being recognized as a dimension so it seems fine. @agithomas WDYT? |
From the TSDB toolkit output, there seems to be no drop in the documents. |
@agithomas if the dynamic mapping of labels is not making each label a dimension in |
@felixbarny ,can you confirm if the feature is available from 8.14.0 or 8.13.0. When tested with 8.13.0, we observed documents getting dropped, possibly indicating missing feature / functionality. [Reference]. (#9785 (comment)) . |
Tested TSDB enablement with 8.14.0-SNAPSHOT, TSDB works fine in 8.14.0-SNAPSHOT. I can see 140 events in before TSDB and after TSDB. MappingsValues being used: elasticsearch_host = https://localhost:9200 elasticsearch_ca_path = /root/.elastic-package/profiles/default/certs/elasticsearch/ca-cert.pem elasticsearch_user = elastic elasticsearch_pwd = changeme data_stream = metrics-prometheus.collector-default docs_index = 0 settings_mappings_index = Last index of the data stream max_docs = All documents get_overlapping_files = True directory_overlapping_files = overwritten-docs-metrics-prometheus.collector-default display_docs = 10 copy_docs_per_dimension = 2You're testing with version 8.14.0-SNAPSHOT. Testing data stream metrics-prometheus.collector-default. The time series fields for the TSDB index are: Index tsdb-index-enabled successfully created. Copying documents from .ds-metrics-prometheus.collector-default-2024.05.15-000001 to tsdb-index-enabled... |
packages/prometheus/manifest.yml
Outdated
description: Collect metrics from Prometheus servers with Elastic Agent. | ||
type: integration | ||
categories: | ||
- observability | ||
- monitoring | ||
- containers | ||
conditions: | ||
kibana.version: "^8.12.1" | ||
kibana.version: "^8.13.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to update this version to 8.14.0? As in 8.13.0 kibana seems like with the TSDB, seems like the events are getting dropped!
🚀 Benchmarks reportTo see the full report comment with |
/test |
1 similar comment
/test |
After bumping to
|
0e95b3d
to
25446ae
Compare
@jsoriano do you have any idea why the kibana container just wont start? The only thing I did is bump kibana version to
|
@gpop63 there is an issue with current 8.14 snapshots, it will be fixed in next snapshots, probably today. More info in elastic/kibana#183334 (comment). |
/test |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, as no document drop reported as part of the included test results.
packages/prometheus/changelog.yml
Outdated
@@ -1,4 +1,9 @@ | |||
# newer versions go on top | |||
- version: "1.15.3" | |||
changes: | |||
- description: Eliminate labels fingerprint for tsdb purposes and use keyword type instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, Remove
instead of Eliminate
would be a better word.
Also, I wonder if it is correct/important to have the phrase - and use the keyword type instead
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query regarding changelog entry added.
Dismissing the review as certain clarity regarding field removal is needed.
packages/prometheus/data_stream/remote_write/elasticsearch/ingest_pipeline/default.yml
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…etheus_fingerprint
Quality Gate passedIssues Measures |
💚 Build Succeeded
History
|
Package prometheus - 1.17.0 containing this change is available at https://epr.elastic.co/search?package=prometheus |
Proposed commit message
Since
8.13.0
8.14.0
we can mark labels field as a dimension directly without having to use the fingerprint processor. Issue was fixed in elastic/elasticsearch#93564.Removed entire pipeline for
collector
data stream as it was only being used for fingerprinting.Data streams affected:
collector
query
remote_write
Checklist
changelog.yml
file.Author's Checklist
How to test this PR locally
Related issues
Screenshots