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

pcp2elasticsearch: support dynamic mapping #2117

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

pauljevans
Copy link
Collaborator

Elasticsearch 7.X removed the concept of mapping types (initially this was depreciated in Elasticsearch 6.X).

Change over to support dynamic mapping by making use of the updated 'POST {server}/{index}/_doc' index API with auto-generated ids by default.

Users can still use the the search type '-p pcp-metric' command line argument on launch to make use of the original pcp-metric mapping name for environments with Elasticsearch 5.X and 6.X, ('POST {server}/{index}/{search_type}' API).

Note: These older versions are considered EOL by Elastic.

This fix addresses github issue #1998.

Elasticsearch 7.X removed the concept of mapping types (initially this was
depreciated in Elasticsearch 6.X).

Change over to support dynamic mapping by making use of the updated
'POST {server}/{index}/_doc' index API with auto-generated ids by default.

Users can still use the the search type '-p pcp-metric' command line argument
on launch to make use of the original pcp-metric mapping name for environments
with Elasticsearch 5.X and 6.X, ('POST {server}/{index}/{search_type}' API).

Note: These older versions are considered EOL by Elastic.

This fix addresses github issue performancecopilot#1998.
@myllynen
Copy link
Contributor

Thanks! I haven't followed Elasticsearch recently but based on your explanation this certainly makes sense.

However, looking at the CI runs were some of the related QA test cases skipped? Have you checked locally is there any QA impact with this change?

Update QA test 1130 to handle the dynamic mapping changes to the exporter
along with providing an updated out file.

Also commented out socat connection reset check in test due to finding it
was being constantly flagged, even for pcp2elasticsearch version prior to
the dynamic mapping patches.
@pauljevans
Copy link
Collaborator Author

pauljevans commented Jan 13, 2025

Hi Marko,

Apologies for missing the QA test update, have provided in a further commit.

I was originally running into an issue with QA/1130 skipping, i'm assuming that CI was skipping this test also?

It seems that 'socat' was giving errors for connection resets during the test runs ("socat on this platform is not behaving as expected"). This behaviour was also being triggered for me with the current upstream version of the exporter also.

I have commented out the 'socat' connection reset notrun check in the test for now.

Cheers

@myllynen
Copy link
Contributor

Thank you! LGTM now but given that Ken spent a lot of time getting the QA running across different platform let's check with him if there would be any additional feedback - @kmcdonell ? Otherwise I think we could merge this. Thanks.

@kmcdonell
Copy link
Member

I don't have anything to add on the code change, this is Python after all.
To check for QA fallout, run ./check -g pmda.elasticsearch from the qa directory once the PMDA change is installed.

@myllynen myllynen merged commit 994dab5 into performancecopilot:main Jan 15, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants