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

Orchestrator additions for features coming in 8.8 #2181

Merged
merged 5 commits into from
Apr 28, 2023

Conversation

mitodrummer
Copy link
Contributor

  • Have you signed the contributor license agreement? y
  • Have you followed the contributor guidelines? y
  • For proposing substantial changes or additions to the schema, have you reviewed the RFC process? y
  • If submitting code/script changes, have you verified all tests pass locally using make test? y
  • If submitting schema/fields updates, have you generated new artifacts by running make and committed those changes? y
  • Is your pull request against main? Unless there is a good reason otherwise, we prefer pull requests against main and will backport as needed. y
  • Have you added an entry to the CHANGELOG.next.md? y

Copy link
Contributor

@kgeller kgeller left a comment

Choose a reason for hiding this comment

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

These changes seem straightforward, but I hesitate with 8.7 being FF and so close to the release date.

@ebeahan what do you think?

@ebeahan
Copy link
Member

ebeahan commented Mar 27, 2023

Several existing integrations using orchestrator.* fields, like the Kubernetes integration package, have established other conventions for capturing labels and annotations. The changes proposed here seem reasonable, but we should pull some other teams into the discussion to make sure there's alignment around the approach.

@elastic/obs-cloud-monitoring @elastic/obs-cloudnative-monitoring

@ebeahan
Copy link
Member

ebeahan commented Mar 27, 2023

Reading through the k8s docs, these key/values are guaranteed to be strings. Does using flattened make sense over keyword as the field types?

Instead of:

orchestrator.resource.annotation:"key1:value1"

Flattened fields parse out the leaf fields and users would instead query:

orchestrator.resource.annotation.key1:"value1"

@mitodrummer
Copy link
Contributor Author

Reading through the k8s docs, these key/values are guaranteed to be strings. Does using flattened make sense over keyword as the field types?

Instead of:

orchestrator.resource.annotation:"key1:value1"

Flattened fields parse out the leaf fields and users would instead query:

orchestrator.resource.annotation.key1:"value1"

It seems wildcard searches are not possible with flattened? e.g. would "orchestrator.resource.annotation.key1:"partial*" be possible?

@mitodrummer
Copy link
Contributor Author

Seems the recent process.env_vars addition takes the same keyword approach. https://www.elastic.co/guide/en/ecs/current/ecs-process.html#field-process-env-vars

@mitodrummer
Copy link
Contributor Author

I see that the kubernetes integration has fields for annotations and labels, but all those fields seem to be custom and outside of ECS. https://github.com/elastic/integrations/blob/main/packages/kubernetes/data_stream/container/fields/base-fields.yml#L68

@mitodrummer
Copy link
Contributor Author

We are going to test using a "flattened" type for these new fields, and will report back here once we've determined if it's a viable way forward. Thanks!

@gizas
Copy link

gizas commented Apr 4, 2023

I see that the kubernetes integration has fields for annotations and labels, but all those fields seem to be custom and outside of ECS

Indeed those fields are "custom", outside the ECS fields and mapped dynamically as they are discovered. I think to be able to search inside eg. "orchestrator.resource.label": [somelabel*] and be able to find an existing label it is really valuable use case for Kubernetes personas that access resources based on labels.

@mitodrummer
Copy link
Contributor Author

I see that the kubernetes integration has fields for annotations and labels, but all those fields seem to be custom and outside of ECS

Indeed those fields are "custom", outside the ECS fields and mapped dynamically as they are discovered. I think to be able to search inside eg. "orchestrator.resource.label": [somelabel*] and be able to find an existing label it is really valuable use case for Kubernetes personas that access resources based on labels.

Yea, if we go the "flattened" route, I guess we lose ability to do wildcard searches on label/annotation "keys". If kibana search bars autocomplete the key values, perhaps that is a non issue, though maybe is less flexible for protection rules etc..

@gsantoro
Copy link
Contributor

gsantoro commented Apr 6, 2023

Hello,

It seems wildcard searches are not possible with flattened? e.g. would "orchestrator.resource.annotation.key1:"partial*" be possible?

I just want to point out that the docs mention that you can't do wildcard search on field keys not field values. Like in the following extract from here

When querying, it is not possible to refer to field keys using wildcards, as in { "term": {"labels.time*": 1541457010}}

@mitodrummer
Copy link
Contributor Author

After testing the "flattened" type. It seems to have too many limitations for our use cases.
a) EQL won't allow it's use in rules
b) the keys of the object aren't autocompleted in kibana searches.
c) aggregations don't seem to be supported.

I think the PR as it stands (array of keywords e.g ["key:value", "key2:value2"] is the most flexible for our use case. cc @ebeahan

image

@mitodrummer mitodrummer requested a review from a team April 28, 2023 17:36
@ebeahan
Copy link
Member

ebeahan commented Apr 28, 2023

I think the PR as it stands (array of keywords e.g ["key:value", "key2:value2"] is the most flexible for our use case. cc

Sorry for the delay. Understood that flattened doesn't sound like the right fit here due to the limitations.

No objections to the changes from the ECS perspective, and @gizas supported the approach here.

@kgeller we're past the 8.8 FF, but I suspect there's low risk to still including these changes into ECS 8.8. WDYT?

@mitodrummer mitodrummer merged commit f9a25f5 into elastic:main Apr 28, 2023
@norrietaylor
Copy link
Member

🥳

kgeller pushed a commit to kgeller/ecs that referenced this pull request May 1, 2023
* new fields added to orchestrator fieldset

* build artifacts

---------

Co-authored-by: Eric Beahan <eric.beahan@elastic.co>
(cherry picked from commit f9a25f5)

# Conflicts:
#	experimental/generated/csv/fields.csv
#	generated/csv/fields.csv
@kgeller
Copy link
Contributor

kgeller commented May 1, 2023

💚 All backports created successfully

Status Branch Result
8.8

Questions ?

Please refer to the Backport tool documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants