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

Add memory_region to api events #445

Merged
merged 5 commits into from
Nov 8, 2023
Merged

Conversation

jdu2600
Copy link
Contributor

@jdu2600 jdu2600 commented Oct 26, 2023

Add missing [Target.]process.Ext.memory_region fields to api events.
These optional fields were introduced in 8.11.0

@jdu2600 jdu2600 requested a review from a team as a code owner October 26, 2023 05:45
@elasticmachine
Copy link
Contributor

elasticmachine commented Oct 26, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-11-06T02:14:10.090+0000

  • Duration: 7 min 25 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@jdu2600 jdu2600 changed the base branch from main to 8.11 October 26, 2023 06:01
@jdu2600
Copy link
Contributor Author

jdu2600 commented Oct 27, 2023

🤔 Why do the generated files from my build have different whitespace to the existing files?
[Update] Rebuilt with parallelism... and the differences went away.

Copy link
Contributor

@gabriellandau gabriellandau left a comment

Choose a reason for hiding this comment

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

Code LGTM but red build.

@jdu2600
Copy link
Contributor Author

jdu2600 commented Oct 28, 2023

Code LGTM but red build.

The five Required items are green - which is sufficient I believe.

The failures look like the same KeyError: 'kibana.version' as last month - which is part of the buildkite migration.

@gabriellandau
Copy link
Contributor

I'm not going to block a merge over this, but could you follow up with a PR that adds the new fields to the sample API event so their inclusion in enforced by make static-test?

@jdu2600
Copy link
Contributor Author

jdu2600 commented Oct 31, 2023

api/sample_event.json

That's a credential_access API event sample - which I didn't want to overwrite.
I looked at that in my last PR, but couldn't see a way to add samples for all of the new API event variants.

@gabriellandau
Copy link
Contributor

That's a credential_access API event sample - which I didn't want to overwrite.

They don't have to be real events with real meaning. The point is just to exercise the schema validation code.

@ferullo
Copy link
Contributor

ferullo commented Oct 31, 2023

Should this be targeting 8.11 or main? Would we really release an updated endpoint package for 8.11?

@gabriellandau
Copy link
Contributor

@jdu2600 the data's already there but not in the mapping, right? If so, that's arguably a bug.

@pzl
Copy link
Member

pzl commented Nov 2, 2023

Echoing @gabriellandau, I would love to see values for these fields added to the sample file.

The sample_event.json file does not need to semantically make any sense. It is for automated testing only to validate that a representative value for any given field, matches what the typing is set in the data stream mapping. The end result, is that it will look like a union of every possible event sample in one document, yes. That is OK here.

Buildkite migration is blocked, so it's in purgatory still, will be red until a dependency is fixed. It can be safely ignored. The rest of the statuses are green.

It seems like this is entirely additions, so backporting is not out of the question for 8.11. Just ping the defend workflows team if we need to cut a release

@jdu2600
Copy link
Contributor Author

jdu2600 commented Nov 3, 2023

Updated the sample file (including fields added in #427).

@pzl
Copy link
Member

pzl commented Nov 3, 2023

we should merge this to main, and then it's a manual backport step to 8.11 branch

@jdu2600 jdu2600 changed the base branch from 8.11 to main November 3, 2023 13:30
@jdu2600
Copy link
Contributor Author

jdu2600 commented Nov 6, 2023

I've re-targeted this PR to main.

@jdu2600 jdu2600 merged commit 3292cf4 into main Nov 8, 2023
6 of 8 checks passed
@jdu2600 jdu2600 deleted the endpoint_memory_region_hash branch November 8, 2023 06:40
pzl pushed a commit that referenced this pull request Dec 11, 2023
* add memory_region to api

* add parameter variants

* add memory_region to sample API event

* add generated file

* force api.yaml to regenerate
@elasticmachine
Copy link
Contributor

Package endpoint - 8.12.0 containing this change is available at https://epr.elastic.co/search?package=endpoint

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.

5 participants