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.

Sign up for GitHub

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

Jump to bottom

[EEM] Update the builtin service entity definition #187021

Merged
merged 29 commits into from
Jul 16, 2024
Merged

[EEM] Update the builtin service entity definition #187021

merged 29 commits into from
Jul 16, 2024

Conversation

klacabane
Copy link
Contributor

@klacabane klacabane commented Jun 26, 2024 •
edited
Loading

Summary

This change includes updates to the builtin service definition:

  • removal of the high cardinality metadata fields until we have a solution in place
  • fetching of the metrics from the preaggregated apm metrics
  • fixed metrics aggregations
  • increased history transform frequency and delay to cover for delayed ingestion

@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@klacabane klacabane marked this pull request as ready for review July 4, 2024 00:03
@klacabane klacabane requested a review from a team as a code owner July 4, 2024 00:03
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Jul 4, 2024
@klacabane klacabane added the release_note:skip Skip the PR/issue when compiling release notes label Jul 4, 2024
@miltonhultgren miltonhultgren added the Feature:EEM Elastic Entity Model label Jul 4, 2024
@kpatticha kpatticha mentioned this pull request Jul 4, 2024
Merged
@miltonhultgren miltonhultgren changed the title [eem] update builtin service definition [EEM] Update the builtin service entity definition Jul 5, 2024
@miltonhultgren miltonhultgren requested a review from kpatticha July 5, 2024 16:29
kpatticha
kpatticha reviewed Jul 9, 2024
View reviewed changes
x-pack/plugins/observability_solution/entity_manager/server/lib/entities/built_in/services.ts
],
},
{
name: 'logRate',
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this track the logRate per minute? If so, can we rename it to make it more clear? ex logRatePerMinute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every aggregation would be per minute here that's why I didn't specify it, do you think we should add the suffix to all ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for yes, we had some discussion about this in the API guidelines document.
Adding the unit aligns more with how ECS names fields and makes it clearer for the consumer what to expect.
It also means that if we want to change the unit we need to consider that a breaking change (or add a new field in a different unit), which is correct since the users might do further computation assuming the unit is correct.

Copy link
Contributor Author

@klacabane klacabane Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not convinced from a dx standpoint, if we update the definition to aggregate over different periods or if we introduce different aggregation (ie 1m, 5m, 30m) the code relying on these aggregations would quite suffer. I'd rather add a different field that would specify the metrics periods like metricset.period. as a working example the apm service_transaction keep the same metric field names regardless of the duration of aggregation

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand a bit on how the relying could would be affected by such a change? I can't see how that would be a heavy downstream change but sure, compared to using a computed value anything seems more but I think the value is that it's more clear for the consumers (end user or Kibana apps) and I'd favor that over our own DX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code needs to change because we introduce new fields. it's just redundant information

kpatticha added a commit that referenced this pull request Jul 10, 2024
@kpatticha @kibanamachine @smith
[APM] Elastic Entity Enablement (#187593)
b682833 
## Summary
closes 
1. elastic/observability-dev#3724 (text
update)
2. elastic/observability-dev#3722 (link to
survey)
3. #187568

Basic workflow for enabling EEM in service inventory view 

The empty states will be handled in different PR 


- When the user clicks on "try new" we set up the builtin service
entities. There is an open PR
#187021 to update the definition.
[1]





### with the right permissions 


https://github.com/elastic/kibana/assets/3369346/2d76d9ca-dc1d-417b-8f06-f2d86b75a32f




### Without the right permissions 


https://github.com/elastic/kibana/assets/3369346/2f2af57c-a95a-4360-b667-1eceb7229d6c


## NOTE  
[1] if you test this PR locally the metrics will be N/A. you need to
update
https://github.com/elastic/kibana/blob/bf45ddd9f4c48acea5fac61d07012110bd4f0f4b/x-pack/plugins/observability_solution/entity_manager/server/lib/entities/built_in/services.ts
. Unless the PR 187021 is merged

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Nathan L Smith <nathan.smith@elastic.co>
@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 15, 2024 •
edited
Loading

⏳ Build in-progress

History

@klacabane klacabane merged commit 393375a into elastic:main Jul 16, 2024
22 checks passed
@kibanamachine kibanamachine added v8.16.0 backport:skip This commit does not require backporting labels Jul 16, 2024
This was referenced Jul 19, 2024
Closed
Merged
kpatticha added a commit that referenced this pull request Jul 22, 2024
@kpatticha
[APM] Updated eem schema (#188763)
7089f35 
## Summary

closes: #188761

### changes

- identityFields returns only the fields, query directly service name
and service environment from entity document (EEM
[change](#187699))
- Rename `logRatePerMinute` to `logRate` (EEM
[change](#187021))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sorenlouv sorenlouv sorenlouv left review comments

@miltonhultgren miltonhultgren miltonhultgren left review comments

@kpatticha kpatticha kpatticha left review comments

@chrisdistasio chrisdistasio chrisdistasio left review comments

@tommyers-elastic tommyers-elastic tommyers-elastic approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project Feature:EEM Elastic Entity Model release_note:skip Skip the PR/issue when compiling release notes v8.16.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@klacabane @obltmachine @elasticmachine @sorenlouv @miltonhultgren @kpatticha @chrisdistasio @tommyers-elastic @kibanamachine