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

x-pack/plugin/apm-data: map some APM fields as flattened and fix error.grouping_name script #103032

Merged
merged 7 commits into from
Dec 16, 2023

Conversation

axw
Copy link
Member

@axw axw commented Dec 6, 2023

Various fixes to the apm-data plugin:

  • fix the error.grouping_name script to not throw an exception when an error.exception object lacks a message attribute
  • statically map various APM-specific fields as flattened to prevent mapping conflicts

Also expand the YAML REST test coverage.

@elasticsearchmachine elasticsearchmachine added v8.12.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Dec 6, 2023
@axw
Copy link
Member Author

axw commented Dec 6, 2023

Requires #103033 and #103035

axw added 3 commits December 13, 2023 13:04
If error.log.message is an empty string, ignore it.
If error.exception exists but has no message field,
don't throw an exception; ignore null/empty values.

Add YAML REST tests.
@axw axw added >enhancement :Data Management/Data streams Data streams and their lifecycles labels Dec 13, 2023
@elasticsearchmachine
Copy link
Collaborator

Hi @axw, I've created a changelog YAML for you.

@axw axw marked this pull request as ready for review December 13, 2023 06:07
@axw axw requested a review from a team as a code owner December 13, 2023 06:07
@elasticsearchmachine elasticsearchmachine added the Team:Data Management Meta label for data/management team label Dec 13, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@dakrone
Copy link
Member

dakrone commented Dec 14, 2023

Hi @axw, I have a couple of contribution drive-by comments:

  • We try not to force-push PRs once they've been opened publicly, as it makes tracking changes over a period of time much harder for a reviewer, since that history is lost. Usually we merge main (if we need to update a PR to be based on the latest changes in main) which retains the history of the PR. We even have a built-in command for this: commenting @elasticmachine update branch will automatically merge main into your PR.
  • For these, can you also increment the REGISTRY_VERSION, since you're changing the mappings? Even though they may be in the same point release as other changes, on Serverless there are no versions, so we'll rely on the registry version to be able to install the latest templates/policies/etc on upgrade.
  • In the future, can you use a more descriptive title or changelog summary for these PRs? The changelog gets automatically generated from the PR title, which in this case means it'll go into the public docs as "X-pack/plugin/apm-data: minor fixes & tests". We want these changelogs to be as useful as possible for users, so they really know what changed when they go to install 8.13.0. Perhaps something like "Map some APM fields as flattened and fix error.grouping_name script" would be useful for our users when they're evaluating the changelog.

Hope this makes sense, let me know if it doesn't, thanks!

@axw
Copy link
Member Author

axw commented Dec 14, 2023

Thanks for taking a look @dakrone!

We try not to force-push PRs once they've been opened publicly, as it makes tracking changes over a period of time much harder for a reviewer, since that history is lost. Usually we merge main (if we need to update a PR to be based on the latest changes in main) which retains the history of the PR. We even have a built-in command for this: commenting @elasticmachine update branch will automatically merge main into your PR.

Got it. Just to be sure, this applies even to draft PRs? (It was draft when I did that.)

For these, can you also increment the REGISTRY_VERSION, since you're changing the mappings? Even though they may be in the same point release as other changes, on Serverless there are no versions, so we'll rely on the registry version to be able to install the latest templates/policies/etc on upgrade.

I don't think it's necessary yet -- we've not yet enabled this in serverless (or anywhere else). The templates/pipelines were a little bit broken, and this PR is getting it to a state where it can start to be enabled. WDYT?

In the future, can you use a more descriptive title or changelog summary for these PRs? The changelog gets automatically generated from the PR title, which in this case means it'll go into the public docs as "X-pack/plugin/apm-data: minor fixes & tests". We want these changelogs to be as useful as possible for users, so they really know what changed when they go to install 8.13.0. Perhaps something like "Map some APM fields as flattened and fix error.grouping_name script" would be useful for our users when they're evaluating the changelog.

Understood, sorry! Will endeavour to make them more meaningful.

@carsonip carsonip self-requested a review December 14, 2023 09:55
@axw axw changed the title x-pack/plugin/apm-data: minor fixes & tests x-pack/plugin/apm-data: map some APM fields as flattened and fix error.grouping_name script Dec 14, 2023
@axw
Copy link
Member Author

axw commented Dec 14, 2023

@elasticmachine update branch

@dakrone
Copy link
Member

dakrone commented Dec 15, 2023

Got it. Just to be sure, this applies even to draft PRs? (It was draft when I did that.)

This is more personal taste (the no-force-merge rule is mostly for non-draft PRs), so I think this one in particular depends on the person.

I don't think it's necessary yet -- we've not yet enabled this in serverless (or anywhere else). The templates/pipelines were a little bit broken, and this PR is getting it to a state where it can start to be enabled. WDYT?

Ahh okay, I hadn't realized this wasn't enabled in Serverless yet, can you point me to where that flag is? Or an issue I can watch for where it'll be enabled eventually?

Thanks!

@axw
Copy link
Member Author

axw commented Dec 16, 2023

Ahh okay, I hadn't realized this wasn't enabled in Serverless yet, can you point me to where that flag is? Or an issue I can watch for where it'll be enabled eventually?

The flag to enable the plugin is:

/** Setting for enabling or disabling APM Data. Defaults to false. */
public static final Setting<Boolean> APM_DATA_ENABLED = Setting.boolSetting(
"xpack.apm_data.enabled",
false,
Setting.Property.NodeScope
);

There's no specific issue yet for enabling this in serverless, but the general issue for tracking related changes is elastic/apm-server#11529. I've written down the rough steps that we'll take to roll out the changes, which I expect we'll make issues out of later.

@axw
Copy link
Member Author

axw commented Dec 16, 2023

@elasticmachine update branch

@axw axw added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 16, 2023
@elasticsearchmachine elasticsearchmachine merged commit 7620902 into elastic:main Dec 16, 2023
15 checks passed
@axw axw deleted the apmdata-fixes branch December 16, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Data Management/Data streams Data streams and their lifecycles >enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team Team:Data Management Meta label for data/management team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants