[APM] Check if metric fields exist #145348

kpatticha · 2022-11-16T12:18:23Z

Fixes the error

"script_stack": [
            "[email protected]/org.elasticsearch.index.fielddata.ScriptDocValues.throwIfEmpty(ScriptDocValues.java:92)",
            "[email protected]/org.elasticsearch.index.fielddata.ScriptDocValues$Longs.get(ScriptDocValues.java:110)",
            "[email protected]/org.elasticsearch.index.fielddata.ScriptDocValues$Longs.getValue(ScriptDocValues.java:105)",
            """total = useCgroupLimit ? doc[limitKey].value : doc['system.memory.total'].value;

    double """,
            "                                                                         ^---- HERE"

connected https://github.com/elastic/sdh-apm/issues/765 (internal)

sorenlouv · 2022-11-16T13:01:27Z

x-pack/plugins/apm/server/routes/metrics/by_agent/shared/memory/index.ts

-      This number represents the max possible value for the limit field.
-    */
-    double CGROUP_LIMIT_MAX_VALUE = 9223372036854771712L;
+    if(doc.containsKey('${METRIC_CGROUP_MEMORY_LIMIT_BYTES}') && doc.containsKey('${METRIC_CGROUP_MEMORY_USAGE_BYTES}')){


What about doc['${METRIC_SYSTEM_TOTAL_MEMORY}'] ?

ah I was trying different things and forgot to add the one that was causing issue 🙈
good catch!

I can get rid of METRIC_CGROUP_MEMORY_LIMIT_BYTES as it's already checking if it exists in this line https://github.com/elastic/kibana/pull/145348/files/41f4588a1e4a64e71e909d303d284cb8eaece403#diff-741ca25328532c18284b64412f95043bb968f82b1ca17fc64193317d41337460R75

@kpatticha can you reach out to the agents or server devs to figure out which fields are available in what scenarios? I'm hoping we can figure out something like "if METRIC_CGROUP_MEMORY_LIMIT_BYTES is available, then METRIC_CGROUP_MEMORY_USAGE_BYTES must be too, and if it's not available, we should use METRIC_SYSTEM_TOTAL_MEMORY, which is always available". Doesn't necessarily have to happen in this PR but I feel like with each change we understand how memory usage is recorded less.

sure I can try that, it will be really useful 👍

I started gathering information on how system memory work here but it should not block this PR

elasticmachine · 2022-11-17T19:50:07Z

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

kpatticha · 2022-11-17T20:06:09Z

@elasticmachine merge upstream

kpatticha · 2022-11-21T09:42:47Z

@elasticmachine merge upstream

kpatticha · 2023-03-28T00:54:12Z

@elasticmachine merge upstream

kpatticha · 2023-03-28T02:49:45Z

@elasticmachine merge upstream

kpatticha · 2023-03-29T09:21:13Z

@elasticmachine merge upstream

x-pack/test/apm_api_integration/tests/service_overview/instances_main_statistics.spec.ts

dgieselaar · 2023-03-30T14:54:31Z

x-pack/plugins/apm/server/routes/metrics/by_agent/shared/memory/index.ts

-      This number represents the max possible value for the limit field.
-    */
-    double CGROUP_LIMIT_MAX_VALUE = 9223372036854771712L;
+    if(doc.containsKey('${METRIC_SYSTEM_TOTAL_MEMORY}') && doc.containsKey('${METRIC_CGROUP_MEMORY_USAGE_BYTES}')){


I'm not sure if this is the way to go about it... will this not unnecessarily return null in some cases? In all honesty it's a little hard to wrap my head around.

I'm putting the PR back to draft. I've noticed recent changes in memory I need to take into consideration. https://github.com/elastic/kibana/blob/main/x-pack/plugins/apm/server/routes/metrics/by_agent/shared/memory/index.ts#L134-L179

kpatticha · 2023-04-13T09:32:44Z

note for me

Another case was reported when using APM dotnet agent. 8.7.2 is in 2 weeks

sorenlouv · 2023-04-17T07:36:24Z

@kpatticha Did you talk to the server/agent folks about this one?

kpatticha · 2023-04-17T08:09:51Z

@sqren yes.

In .NET on macOS they don’t collect system memory usage
Also in .Net cgroup metrics is linux-only - https://github.com/elastic/apm-agent-dotnet/blob/main/src/Elastic.Apm/Metrics/MetricsProvider/CgroupMetricsProvider.cs#L82
Nodejs doesn't support cgroup metrics yet

so generally there is no guarantee that any of the system metrics will be available

Co-authored-by: Søren Louv-Jansen <[email protected]>

…inless-script

kibana-ci · 2023-04-27T12:55:15Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 30b0da1

Failed CI Steps

APM Cypress Tests #1

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	17	19	+2
`securitySolution`	399	402	+3
total			+5

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	18	20	+2
`securitySolution`	479	482	+3
total			+5

History

💔 Build #123832 failed e8b1a56
💔 Build #123803 failed 28c5289
💔 Build #120732 failed 05aa070
💔 Build #120722 failed 14fadf4
💛 Build #120299 was flaky ab42af5

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

cc @kpatticha

Fixes the error ``` "script_stack": [ "[email protected]/org.elasticsearch.index.fielddata.ScriptDocValues.throwIfEmpty(ScriptDocValues.java:92)", "[email protected]/org.elasticsearch.index.fielddata.ScriptDocValues$Longs.get(ScriptDocValues.java:110)", "[email protected]/org.elasticsearch.index.fielddata.ScriptDocValues$Longs.getValue(ScriptDocValues.java:105)", """total = useCgroupLimit ? doc[limitKey].value : doc['system.memory.total'].value; double """, " ^---- HERE" ``` connected elastic/sdh-apm#765 (internal) --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Søren Louv-Jansen <[email protected]> (cherry picked from commit 98165d2)

kibanamachine · 2023-04-27T13:03:45Z

💔 Some backports could not be created

Status	Branch	Result
❌	8.7	Backport failed because of merge conflicts
✅	8.8

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 145348

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `8.8`: - [[APM] Check if metric fields exist (#145348)](#145348)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Katerina Patticha <[email protected]>

sorenlouv · 2023-05-03T19:04:05Z

x-pack/plugins/apm/server/routes/metrics/by_agent/shared/memory/index.ts

+export const systemMemoryFilter = {
+  bool: {
+    filter: [
+      { exists: { field: METRIC_SYSTEM_FREE_MEMORY } },
+      { exists: { field: METRIC_SYSTEM_TOTAL_MEMORY } },
+    ],
+  },
+};
+
 export const percentSystemMemoryUsedScript = {


@kpatticha any reason for the difference in naming "systemMemoryFilter" and "percentSystemMemoryUsedScript"? What about "systemMemoryFilter" and "systemMemoryScript"?

Ideally I think they should be co-located in an object:

const systemMemory = { filter: {...}, script: {...} }

This will provide a strong signal that they belong together, and will make it less likely that somebody changing the code will make changes to the filter and not the script, and vice-versa.

@sqren I can't remember any particular reason for the naming 🤔 .

As for the object, that's a really good point and I think we initially discussed doing it like that but somehow I missed it.

Let me fix it in a follow-up PR

sorenlouv · 2023-05-03T19:04:49Z

x-pack/plugins/apm/server/routes/metrics/by_agent/shared/memory/index.ts

+export const cgroupMemoryFilter = {
+  bool: {
+    filter: [{ exists: { field: METRIC_CGROUP_MEMORY_USAGE_BYTES } }],
+    should: [
+      { exists: { field: METRIC_CGROUP_MEMORY_LIMIT_BYTES } },
+      { exists: { field: METRIC_SYSTEM_TOTAL_MEMORY } },
+    ],
+    minimum_should_match: 1,
+  },
+};
+
 export const percentCgroupMemoryUsedScript = {


Same as above (naming)

follow up: #145348 (comment)

sorenlouv reviewed Nov 16, 2022

View reviewed changes

kpatticha marked this pull request as ready for review November 17, 2022 19:42

kpatticha requested a review from a team as a code owner November 17, 2022 19:42

botelastic bot added the Team:APM All issues that need APM UI Team support label Nov 17, 2022

kpatticha added release_note:skip Skip the PR/issue when compiling release notes v8.7.0 labels Nov 17, 2022

kpatticha mentioned this pull request Nov 17, 2022

[APM] Document how system memory usage is recorded in apm #145621

Closed

1 task

kpatticha requested review from sorenlouv and dgieselaar November 21, 2022 10:54

sorenlouv added v8.8.0 release_note:fix v8.7.1 and removed v8.7.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 22, 2023

achyutjhunjhunwala reviewed Mar 29, 2023

View reviewed changes

x-pack/test/apm_api_integration/tests/service_overview/instances_main_statistics.spec.ts Show resolved Hide resolved

kpatticha force-pushed the painless-script branch 2 times, most recently from a84fcc0 to 87fa356 Compare March 29, 2023 12:01

kpatticha requested a review from achyutjhunjhunwala March 29, 2023 12:03

dgieselaar reviewed Mar 30, 2023

View reviewed changes

kpatticha marked this pull request as draft April 3, 2023 08:04

kpatticha self-assigned this Apr 3, 2023

Fix gb to bytes

d8fe9df

kpatticha force-pushed the painless-script branch from 774d47e to d8fe9df Compare April 18, 2023 14:25

Simplify cgroupMemoryFilter

05aa070

Co-authored-by: Søren Louv-Jansen <[email protected]>

sorenlouv approved these changes Apr 18, 2023

View reviewed changes

kpatticha added 2 commits April 24, 2023 12:10

Merge branch 'main' of github.com:elastic/kibana into painless-script

0c809b5

Merge branch 'main' of github.com:elastic/kibana into painless-script

d9a759a

kpatticha enabled auto-merge (squash) April 27, 2023 09:10

kpatticha added the backport:prev-major Backport to (8.x, 8.17, 8.16) the previous major branch and other branches in development label Apr 27, 2023

kpatticha added 3 commits April 27, 2023 11:13

Fix test

85e98bf

Merge branch 'main' of github.com:elastic/kibana into painless-script

78415a5

Merge branch 'painless-script' of github.com:kpatticha/kibana into pa…

28c5289

…inless-script

kpatticha added 3 commits April 27, 2023 12:06

Update snapshots

e8b1a56

Fix the cgroupMemoryFilter query

0cbecfc

Revert the change

30b0da1

kpatticha merged commit 98165d2 into elastic:main Apr 27, 2023

kibanamachine added the v8.9.0 label Apr 27, 2023

kibanamachine mentioned this pull request Apr 27, 2023

[8.8] [APM] Check if metric fields exist (#145348) #156006

Merged

sorenlouv reviewed May 3, 2023

View reviewed changes

kpatticha mentioned this pull request May 4, 2023

[APM] co-locate system and cgroup memory filter and script #156654

Merged

kpatticha added a commit that referenced this pull request May 4, 2023

[APM] co-locate system and cgroup memory filter and script (#156654)

bc886e1

follow up: #145348 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Check if metric fields exist #145348

[APM] Check if metric fields exist #145348

kpatticha commented Nov 16, 2022 •

edited by kibanamachine

Loading

sorenlouv Nov 16, 2022

kpatticha Nov 16, 2022

kpatticha Nov 16, 2022

dgieselaar Nov 17, 2022

kpatticha Nov 17, 2022

kpatticha Mar 29, 2023

elasticmachine commented Nov 17, 2022

kpatticha commented Nov 17, 2022

kpatticha commented Nov 21, 2022

kpatticha commented Mar 28, 2023

kpatticha commented Mar 28, 2023

kpatticha commented Mar 29, 2023

dgieselaar Mar 30, 2023

kpatticha Apr 3, 2023

kpatticha commented Apr 13, 2023

sorenlouv commented Apr 17, 2023

kpatticha commented Apr 17, 2023

kibana-ci commented Apr 27, 2023

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented Apr 27, 2023

sorenlouv May 3, 2023 •

edited

Loading

kpatticha May 4, 2023

kpatticha May 4, 2023

sorenlouv May 3, 2023

[APM] Check if metric fields exist #145348

[APM] Check if metric fields exist #145348

Conversation

kpatticha commented Nov 16, 2022 • edited by kibanamachine Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 17, 2022

kpatticha commented Nov 17, 2022

kpatticha commented Nov 21, 2022

kpatticha commented Mar 28, 2023

kpatticha commented Mar 28, 2023

kpatticha commented Mar 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpatticha commented Apr 13, 2023

sorenlouv commented Apr 17, 2023

kpatticha commented Apr 17, 2023

kibana-ci commented Apr 27, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented Apr 27, 2023

💔 Some backports could not be created

Manual backport

Questions ?

sorenlouv May 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpatticha commented Nov 16, 2022 •

edited by kibanamachine

Loading

sorenlouv May 3, 2023 •

edited

Loading