Conversation
|
💚 CLA has been signed |
| // Go profiles report samples.count, Node.js profiles report sample.count. | ||
| // We use samples.count for both so we can aggregate on one field. |
There was a problem hiding this comment.
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).
7438546 to
664c54e
Compare
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
Implements support for Node.js profiles, by normalizing sample.count to samples.count and adding wall.microseconds as a supported value type.
664c54e to
dfb7022
Compare
Codecov Report
@@ 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
|
| - name: wall | ||
| type: group | ||
| fields: | ||
| - name: us |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
jenkins run the tests please |
axw
left a comment
There was a problem hiding this comment.
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 <dario.gieselaar@elastic.co>
…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
|
I'm going to remove this from the test plan since it's currently only intended for internal consumption. |
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
For functional changes, consider:
How to test these changes
Related issues