feat: runtime metrics #375

seemk · 2021-10-28T13:36:40Z

Description

Add runtime metrics collection in a similar fashion as is done by signalfx-nodejs-collect. However signalfx-nodejs-collect used 2 different native extensions to gather event loop and GC metrics. For event loop metric only the whole cycle duration (including the poll phase) was reported as a metric, which is quite useless. It should have measured event loop lag, which is the whole event loop cycle minus the time it took for IO, timer polling (i.e. no CPU used). Besides this signalfx-nodejs-collect had a lot of dead code regarding events, which were never sent (e.g. on each GC cycle). So the decision was to drop signalfx-nodejs-collect for a custom native extension.

Currently the exporting step will still use the SignalFx client.

List of metrics exported (see signalfx-nodejs-collect for the previous list):

nodejs.memory.heap.total (gauge, bytes) - Heap total via process.memoryUsage().heapTotal
nodejs.memory.heap.used (gauge, bytes) - Heap used via process.memoryUsage().heapUsed
nodejs.memory.rss (gauge, bytes) - Resident set size via process.memoryUsage().rss
nodejs.memory.gc.size (cumulative_counter, bytes) - Memory collected by the garbage collector
nodejs.memory.gc.pause (cumulative_counter, nanoseconds) - Time spent doing GC
nodejs.memory.gc.count (cumulative_counter, count) - Number of times GC ran. (signalfx-nodejs-collect: nodejs.memory.gc.total)
nodejs.event_loop.lag.max (gauge, nanoseconds) - Max event loop lag (signalfx-nodejs-collect: nodejs.event_loop.max)
nodejs.event_loop.lag.min (gauge, nanoseconds) - Min event loop lag (signalfx-nodejs-collect: nodejs.event_loop.min)

Environment variables introduced:

SPLUNK_METRICS_ENABLED (default: 'false') - Metrics are opt-in
SPLUNK_METRICS_ENDPOINT (default: http://localhost:9943)
SPLUNK_METRICS_EXPORT_INTERVAL (default: 5000)

Misc:

Why getSignalFxClient? Custom metrics will still need to be supported. This PR allows to provide your own SignalFx client to startMetrics, but since we configure it with the same access token anyway, they can just use this helper method to access the currently used client.

Type of change

New feature (non-breaking change which adds functionality)
Documentation change or requires a documentation update

How Has This Been Tested?

Tested manually
Added automated tests

Checklist:

Unit tests have been added/updated
Documentation has been updated
Change file has been generated (npm run change:new)
Delete this branch (after the PR is merged)

codecov-commenter · 2021-10-28T14:04:23Z

Codecov Report

Merging #375 (6f01c1c) into main (6afb5b3) will decrease coverage by 0.54%.
The diff coverage is 91.42%.

@@            Coverage Diff             @@
##             main     #375      +/-   ##
==========================================
- Coverage   93.63%   93.08%   -0.55%     
==========================================
  Files           9       12       +3     
  Lines         330      434     +104     
  Branches       87      104      +17     
==========================================
+ Hits          309      404      +95     
- Misses         21       30       +9

Impacted Files	Coverage Δ
src/options.ts	`98.09% <88.88%> (-0.88%)`	⬇️
src/metrics/index.ts	`90.90% <90.90%> (ø)`
src/instrument.ts	`100.00% <100.00%> (ø)`
src/metrics/memory.ts	`100.00% <100.00%> (ø)`
src/metrics/native/index.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6afb5b3...6f01c1c. Read the comment docs.

happens between and counters are updated

…rics

theletterf · 2021-11-02T16:50:03Z

docs/advanced-config.md

+| Environment variable<br>``startMetrics()`` argument             | Default value           | Support | Notes
+| --------------------------------------------------------------- | ----------------------- | ------- | ---
+| `SPLUNK_METRICS_ENABLED`<br>`enabled`                           | `false`                 | Experimental | Enabled metrics export. See [metrics documentation](metrics.md) for more information.
+| `SPLUNK_METRICS_ENDPOINT`<br>`endpoint`                         | `http://localhost:9943` | Experimental | The SignalFx metrics endpoint to send to.
+| `SPLUNK_METRICS_EXPORT_INTERVAL`<br>`exportInterval`            | `5000`                  | Experimental | The interval, in milliseconds, of metrics collection and exporting.


Perhaps an extra column for the startMetrics argument would make it easier to scan the table.

s/SignalFx/Splunk

Added a link to SignalFx client. Followed the existing doc style and startTracing did not have the extra column either, should I split both of them to separate column?

theletterf · 2021-11-02T16:51:36Z

docs/advanced-config.md

+
+#### Additional `startMetrics` config options
+
+- `signalfx`: A JS object with optional `client` and `dimensions` fields. If you have already setup a SignalFx client with custom configuration, it is possible use this for sending instead of creating, configuring a new one. `dimensions` object adds a pre-defined dimension for each datapoint. The format for `dimensions` is `{key: value, ...}`.


Suggested change

- `signalfx`: A JS object with optional `client` and `dimensions` fields. If you have already setup a SignalFx client with custom configuration, it is possible use this for sending instead of creating, configuring a new one. `dimensions` object adds a pre-defined dimension for each datapoint. The format for `dimensions` is `{key: value, ...}`.

- `signalfx`: A JS object with optional `client` and `dimensions` fields. If you have already setup a Splunk client with custom configuration, you can use this for sending instead of creating, configuring a new one. `dimensions` object adds a pre-defined dimension for each datapoint. The format for `dimensions` is `{key: value, ...}`.

Can we rename the setting to "splunk"?

Hmm, explicitly wanted to make it clear that we are dealing with SignalFx Node.js client. What do you think about removing the signalfx field completely? And replace it with 2 fields: client and dimensions?

The old library? Then it makes sense. Just wondering about backward compatibility if the switch to OTel metrics in the future (see Owais' comments).

I added a link to the SignalFx client there and mentioned that metrics will change once OpenTelemetry becomes available.

owais · 2021-11-02T19:45:15Z

How will this work with Otel? What happens when Otel introduces similar runtime metrics? Are we marking this as unstable for now so we have a path to migrate to Otel implementation later?

Or why aren't we contributing this to otel contrib?

seemk · 2021-11-03T07:54:59Z

How will this work with Otel? What happens when Otel introduces similar runtime metrics? Are we marking this as unstable for now so we have a path to migrate to Otel implementation later?

Or why aren't we contributing this to otel contrib?

Yes, it'll be unstable for now. Runtime metrics are disabled by default for that reason. When OTel semconvs and the metrics SDK is available, there will be a breaking change. And after semconvs are available we can start contributing the module upstream.

owais · 2021-11-03T10:03:28Z

OK. If it is experimental and users understand things can break then it's fine I think. That said, SignalFx never had a single library for metrics and traces. Customers always had to use two different libs. Why wouldn't we let customers use signalfx metrics lib till we have counterparts in otel ready?

theletterf · 2021-11-03T10:43:31Z

I tend to agree with @owais here. What's the take of Ivo on this?

scripts/prebuild-current.js

rauno56

I'd still rather see the native extension have a separate life-cycle in another package, but this works for now.

src/metrics/index.ts

src/metrics/native/module.cpp

test/examples/Dockerfile_test

owais · 2021-11-03T11:33:13Z

We should at least try to add missing conventions to Otel metrics semantic conventions. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/system-metrics.md

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/README.md

ivomagi · 2021-11-03T13:00:38Z

I tend to agree with @owais here. What's the take of Ivo on this?

I think I can restate the goals

Satisfy the need for runtime metrics today, not when Otel metrics SDK is stable and semconvs available
Package the solution in a way where it is easy to adopt
Make sure the means to transition to Otel-world are taken into account and we know what is the upgrade path to Otel semconv and data model and SDK. There is an epic for next PI where this is taken apart in more details.
Provide similar user experience to other instrumentation lis in this aspect (only Java so far)

Whether or not the macro requirements above can be fulfilled with one or two client-facing artifacts is engineering call. Maybe worth a call as well to go over the concerns.

seemk added 6 commits October 25, 2021 23:03

native runtime info collection

40aefdc

metrics sending

d5d4a1a

add tests

341840a

lint

1294097

Merge branch 'main' into metrics

abdaf13

fix: cumulative counters

2b4439b

seemk added 18 commits October 28, 2021 17:08

add deps for node-gyp

9111a96

actually call apk add

7b32332

apk sudo

1215e5a

apk add as the correct user

33fd9da

apk add python3

bc6810e

add binding.gyp to docker image

b2c6de4

test: spin with hrtime

f6e4a33

test: debug output

68e96f3

test: format eventloop test message

c6bb62e

test: copy native module to the correct path

0364607

fix: native builds for packaging

826a244

test: fix getSignalFxClient

ef9f55d

fix: lint

7bfcde6

test: separate e2e Dockerfiles

fb1449c

test: run compilation on windows

61348a8

fix: set metrics disabled by default

df3b092

doc: add docs

ad0098f

Change files

d3c3622

seemk marked this pull request as ready for review November 1, 2021 15:19

seemk requested review from a team as code owners November 1, 2021 15:19

test: remove event loop collection flakiness

e05ae88

seemk added 4 commits November 1, 2021 18:01

Merge branch 'main' into metrics

6f01c1c

Merge branch 'main' into metrics

f879f1c

test: add a small timeout to native counters test to ensure a poll step

a6f1524

happens between and counters are updated

Merge branch 'metrics' of github.com:signalfx/splunk-otel-js into met…

4631d82

…rics

theletterf suggested changes Nov 2, 2021

View reviewed changes

rauno56 reviewed Nov 3, 2021

View reviewed changes

scripts/prebuild-current.js Outdated Show resolved Hide resolved

rauno56 approved these changes Nov 3, 2021

View reviewed changes

src/metrics/index.ts Outdated Show resolved Hide resolved

src/metrics/native/module.cpp Outdated Show resolved Hide resolved

test/examples/Dockerfile_test Outdated Show resolved Hide resolved

seemk added 3 commits November 4, 2021 12:47

docs: improve advanced-config wording

c7ca07c

docs: mention future changes

4c1ccb2

refactor: add missing newlines, remove the need for a loop

f7eca66

theletterf approved these changes Nov 5, 2021

View reviewed changes

seemk merged commit 668ffd8 into main Nov 11, 2021

seemk deleted the metrics branch November 11, 2021 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: runtime metrics #375

feat: runtime metrics #375

seemk commented Oct 28, 2021 •

edited

Loading

codecov-commenter commented Oct 28, 2021 •

edited

Loading

theletterf Nov 2, 2021

theletterf Nov 2, 2021

seemk Nov 4, 2021

theletterf Nov 2, 2021

seemk Nov 3, 2021

theletterf Nov 3, 2021

seemk Nov 4, 2021

owais commented Nov 2, 2021

seemk commented Nov 3, 2021

owais commented Nov 3, 2021

theletterf commented Nov 3, 2021

rauno56 left a comment

owais commented Nov 3, 2021 •

edited

Loading

ivomagi commented Nov 3, 2021


		#### Additional `startMetrics` config options

		- `signalfx`: A JS object with optional `client` and `dimensions` fields. If you have already setup a SignalFx client with custom configuration, it is possible use this for sending instead of creating, configuring a new one. `dimensions` object adds a pre-defined dimension for each datapoint. The format for `dimensions` is `{key: value, ...}`.

feat: runtime metrics #375

feat: runtime metrics #375

Conversation

seemk commented Oct 28, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

codecov-commenter commented Oct 28, 2021 • edited Loading

Codecov Report

theletterf Nov 2, 2021

Choose a reason for hiding this comment

theletterf Nov 2, 2021

Choose a reason for hiding this comment

seemk Nov 4, 2021

Choose a reason for hiding this comment

theletterf Nov 2, 2021

Choose a reason for hiding this comment

seemk Nov 3, 2021

Choose a reason for hiding this comment

theletterf Nov 3, 2021

Choose a reason for hiding this comment

seemk Nov 4, 2021

Choose a reason for hiding this comment

owais commented Nov 2, 2021

seemk commented Nov 3, 2021

owais commented Nov 3, 2021

theletterf commented Nov 3, 2021

rauno56 left a comment

Choose a reason for hiding this comment

owais commented Nov 3, 2021 • edited Loading

ivomagi commented Nov 3, 2021

seemk commented Oct 28, 2021 •

edited

Loading

codecov-commenter commented Oct 28, 2021 •

edited

Loading

owais commented Nov 3, 2021 •

edited

Loading