Support for Node.js profiles #4728

dgieselaar · 2021-02-14T12:01:37Z

Implements support for Node.js profiles, by normalizing sample.count to samples.count and adding wall.microseconds as a supported value type (Node.js pprof profiles don't support CPU time, only wall time).

Maybe a little premature but seems like a low-risk change?

Would appreciate any guidance on what tests should be added (and where).

Checklist

Update CHANGELOG.asciidoc
Documentation has been updated

For functional changes, consider:

Is it observable through the addition of either logging or metrics?
Is its use being published in telemetry to enable product improvement?
Have system tests been added to avoid regression?

How to test these changes

Related issues

cla-checker-service · 2021-02-14T12:01:39Z

💚 CLA has been signed

dgieselaar · 2021-02-14T12:03:59Z

model/profile.go

+		// Go profiles report samples.count, Node.js profiles report sample.count.
+		// We use samples.count for both so we can aggregate on one field.


Convention says that the first value type is always the number of samples collected. However, for heap profiles this field is alloc_objects.count, which is explicitly aggregated on in fetcher.go (and passed to the pprof UI).

apmmachine · 2021-02-14T12:07:06Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #4728 updated
Start Time: 2021-02-18T07:45:42.311+0000
Duration: 47 min 13 sec
Commit: c123d91

Test stats 🧪

Test	Results
Failed	0
Passed	4751
Skipped	124
Total	4875

Trends 🧪

Implements support for Node.js profiles, by normalizing sample.count to samples.count and adding wall.microseconds as a supported value type.

codecov-io · 2021-02-14T12:27:18Z

Codecov Report

Merging #4728 (c123d91) into master (5042806) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4728      +/-   ##
==========================================
+ Coverage   76.80%   76.81%   +0.01%     
==========================================
  Files         166      166              
  Lines       10239    10244       +5     
==========================================
+ Hits         7864     7869       +5     
  Misses       2375     2375

Impacted Files	Coverage Δ
model/profile.go	`100.00% <100.00%> (ø)`

axw · 2021-02-15T01:14:15Z

model/profile/_meta/fields.yml

+        - name: wall
+          type: group
+          fields:
+            - name: us


It would be nice to have a single base unit -- on-CPU time is recorded as profile.cpu.ns. In theory if we stored in nanoseconds then we could run into overflows when aggregating over a large cluster (hundreds) of service instances, over a significant time period (months to years).

Probably something to consider later, when we firm all of this up.

axw

Nice, thanks!

Maybe a little premature but seems like a low-risk change?

Perfectly fine and low-risk indeed.

Would appreciate any guidance on what tests should be added (and where).

For now I think it would be good enough to modify model/profile_test.go, adding sample.count and wall.microseconds samples and checking they're normalised appropriately.

We can add more extensive functional tests later.

…odejs-profiles

dgieselaar · 2021-02-17T19:52:13Z

jenkins run the tests please

axw

LGTM! Can you please add an entry to changelogs/head.asciidoc? Then it's good to merge.

* Support for Node.js profiles Implements support for Node.js profiles, by normalizing sample.count to samples.count and adding wall.microseconds as a supported value type. # Conflicts: # changelogs/head.asciidoc # include/fields.go

* Support for Node.js profiles Implements support for Node.js profiles, by normalizing sample.count to samples.count and adding wall.microseconds as a supported value type. # Conflicts: # changelogs/head.asciidoc # include/fields.go Co-authored-by: Dario Gieselaar <[email protected]>

…chemas-to-agents * upstream/master: (111 commits) Introduce metricset.name (elastic#4857) processor/otel: test service.version handling (elastic#4853) docs: Add PHP agent information to shared docs (elastic#4740) Script for faster development workflow (elastic#4731) Update to elastic/beats@1b31c26 (elastic#4763) backport: add 7.12 to .backportrc.json (elastic#4807) backport: enable auto-merge on backport PRs (elastic#4777) Support for Node.js profiles (elastic#4728) docs: readds .NET link (elastic#4764) [DOCS] Fixes URLs on Secure communication with APM Agents page (elastic#4746) ci: set proper parameters for the tar step (elastic#4696) docs: add 7.11.1 release notes (elastic#4727) Disable sourcemap upload endpoint when data streams enabled (elastic#4735) Add service name to dataset field (elastic#4674) Update to elastic/beats@ba423212a660 (elastic#4733) sampling: require a default policy (elastic#4729) processor/otel: add unit test for span status (elastic#4734) Add support for consuming OTLP/gRPC metrics (elastic#4722) [apmpackage] Add config options supported in ESS (elastic#4690) Use the apm-server version everywhere* (elastic#4725) ...

…te-schema-json-1 * upstream/master: (111 commits) Introduce metricset.name (elastic#4857) processor/otel: test service.version handling (elastic#4853) docs: Add PHP agent information to shared docs (elastic#4740) Script for faster development workflow (elastic#4731) Update to elastic/beats@1b31c26 (elastic#4763) backport: add 7.12 to .backportrc.json (elastic#4807) backport: enable auto-merge on backport PRs (elastic#4777) Support for Node.js profiles (elastic#4728) docs: readds .NET link (elastic#4764) [DOCS] Fixes URLs on Secure communication with APM Agents page (elastic#4746) ci: set proper parameters for the tar step (elastic#4696) docs: add 7.11.1 release notes (elastic#4727) Disable sourcemap upload endpoint when data streams enabled (elastic#4735) Add service name to dataset field (elastic#4674) Update to elastic/beats@ba423212a660 (elastic#4733) sampling: require a default policy (elastic#4729) processor/otel: add unit test for span status (elastic#4734) Add support for consuming OTLP/gRPC metrics (elastic#4722) [apmpackage] Add config options supported in ESS (elastic#4690) Use the apm-server version everywhere* (elastic#4725) ...

* Support for Node.js profiles Implements support for Node.js profiles, by normalizing sample.count to samples.count and adding wall.microseconds as a supported value type. (cherry picked from commit 5a1318b) # Conflicts: # include/fields.go

axw · 2021-04-28T03:27:47Z

I'm going to remove this from the test plan since it's currently only intended for internal consumption.

dgieselaar commented Feb 14, 2021

View reviewed changes

dgieselaar force-pushed the support-nodejs-profiles branch from 7438546 to 664c54e Compare February 14, 2021 12:05

Support for Node.js profiles

dfb7022

Implements support for Node.js profiles, by normalizing sample.count to samples.count and adding wall.microseconds as a supported value type.

dgieselaar force-pushed the support-nodejs-profiles branch from 664c54e to dfb7022 Compare February 14, 2021 12:07

axw reviewed Feb 15, 2021

View reviewed changes

dgieselaar added 3 commits February 17, 2021 15:40

Merge branch 'master' of github.com:elastic/apm-server into support-n…

927ce23

…odejs-profiles

Test profile model

f7a83d6

Merge branch 'master' of github.com:elastic/apm-server into support-n…

7ecee9d

…odejs-profiles

dgieselaar mentioned this pull request Feb 17, 2021

[APM] Update APM index pattern with added profile fields elastic/kibana#91738

Merged

axw approved these changes Feb 18, 2021

View reviewed changes

Update CHANGELOG

c123d91

axw merged commit 5a1318b into elastic:master Feb 18, 2021

axw mentioned this pull request Feb 20, 2021

[7.x] Support for Node.js profiles (#4728) #4850

Merged

jalvz added test-plan v7.13.0 labels Apr 27, 2021

mergify bot mentioned this pull request Apr 27, 2021

[7.13] Support for Node.js profiles (backport #4728) #5141

Closed

axw removed the test-plan label Apr 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Node.js profiles #4728

Support for Node.js profiles #4728

dgieselaar commented Feb 14, 2021 •

edited

Loading

cla-checker-service bot commented Feb 14, 2021 •

edited

Loading

dgieselaar Feb 14, 2021 •

edited

Loading

apmmachine commented Feb 14, 2021 •

edited

Loading

Build stats

Test stats 🧪

Trends 🧪

codecov-io commented Feb 14, 2021 •

edited

Loading

axw Feb 15, 2021

axw left a comment

dgieselaar commented Feb 17, 2021

axw left a comment

axw commented Apr 28, 2021

		// Go profiles report samples.count, Node.js profiles report sample.count.
		// We use samples.count for both so we can aggregate on one field.

Support for Node.js profiles #4728

Support for Node.js profiles #4728

Conversation

dgieselaar commented Feb 14, 2021 • edited Loading

Checklist

How to test these changes

Related issues

cla-checker-service bot commented Feb 14, 2021 • edited Loading

dgieselaar Feb 14, 2021 • edited Loading

Choose a reason for hiding this comment

apmmachine commented Feb 14, 2021 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

codecov-io commented Feb 14, 2021 • edited Loading

Codecov Report

axw Feb 15, 2021

Choose a reason for hiding this comment

axw left a comment

Choose a reason for hiding this comment

dgieselaar commented Feb 17, 2021

axw left a comment

Choose a reason for hiding this comment

axw commented Apr 28, 2021

dgieselaar commented Feb 14, 2021 •

edited

Loading

cla-checker-service bot commented Feb 14, 2021 •

edited

Loading

dgieselaar Feb 14, 2021 •

edited

Loading

apmmachine commented Feb 14, 2021 •

edited

Loading

codecov-io commented Feb 14, 2021 •

edited

Loading