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

[APM] Collect total number of service groups #142768

Merged

Conversation

kpatticha
Copy link
Contributor

Summary

part of #128621

Collects the total number of service groups.

total field is added in the service_groups object

...
"service_groups": {
   "kuery_fields": [ 
      "service.language.name",
      "agent.name",
      "service.environment"
    ],
  "total": 3
},
...

How to test locally

  1. request GET {KIBANA_HOST}/internal/apm/debug-telemetry
  2. look for service_groups

@kpatticha kpatticha requested a review from a team as a code owner October 5, 2022 15:44
@botelastic botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Oct 5, 2022
@kpatticha kpatticha added release_note:skip Skip the PR/issue when compiling release notes v8.6.0 labels Oct 5, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

Copy link
Contributor

@gbamparop gbamparop left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding tests

@kpatticha kpatticha requested review from a team as code owners October 5, 2022 16:30
@@ -1134,7 +1135,7 @@ export const tasks: TelemetryTask[] = [
const response = await savedObjectsClient.find<SavedServiceGroup>({
type: APM_SERVICE_GROUP_SAVED_OBJECT_TYPE,
page: 1,
perPage: 50,
perPage: MAX_NUMBER_OF_SERVICE_GROUPS,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is actually retrieving the service groups instead of doing a count. Is that not possible with saved objects?

Copy link
Member

Choose a reason for hiding this comment

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

Ah.. I see we retrieve the kuery field. Are we able to reduce the response from ES to just this field then?
(Ideally we'd do a terms agg on kuery to avoid retrieving duplicates).

Copy link
Member

Choose a reason for hiding this comment

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

Doing a terms agg with a size of 500 will also return more unique results (up to 500 to be exact) since it does not include duplicates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the kuery I think we don't want to avoid duplicates. We want to collect all the possible kuery_fields

Copy link
Member

Choose a reason for hiding this comment

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

What is the added value of two kuery strings that are the same? Are you counting the number of times a field is being used?

Copy link
Member

Choose a reason for hiding this comment

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

One question though, does this collect telemetry for all spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why you don't want to avoid kuery duplicates

In order to see which fields are most often used.

there's really no point in having service groups with the exact same kuery as others

that's right but there might be service groups that use same fields but different kuery.

For example 2 service groups

{
  ...
  "groupName": "production java",
  "kuery": "service.environement: 'production' or agent.name:'java' ",
  ...
},
{
  ...
  "groupName": "stage envs",
  "kuery": "service.environement: 'stage' ",
  ...
},

we'll collect

kuery_fields: ['service.environement', 'service.environement', 'agent.name',]

Copy link
Member

Choose a reason for hiding this comment

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

@kpatticha Elasticsearch essentially deduplicates non-unique values anyway. I don't think there's a point in storing a single value multiple times.

But more importantly, can you make sure this telemetry is collected for all spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh! I wasn't aware about this.

😱 that's good catch.

savedObjectsClient.find searches in the default space. Let me fix it in a follow up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

follow up PR - #142856

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

Looks ok. I added a comment but it might be more of an issue with saved objects than anything else.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kpatticha kpatticha merged commit 4197c03 into elastic:main Oct 6, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Oct 6, 2022
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 11, 2022
* [APM] Collect the total number of service groups

* Add unit tests

* Update telemetry mapping

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* update snapshots

Co-authored-by: kibanamachine <[email protected]>
WafaaNasr pushed a commit to WafaaNasr/kibana that referenced this pull request Oct 14, 2022
* [APM] Collect the total number of service groups

* Add unit tests

* Update telemetry mapping

* [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix'

* update snapshots

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants