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

feat(opentelemetry-instrumentation-runtime): initial version #1272

Closed

Conversation

mentos1386
Copy link
Contributor

@mentos1386 mentos1386 commented Nov 6, 2022

Which problem is this PR solving?

Prepares a module for runtime instrumentation, runtime metrics.

Short description of the changes

I have created a new module for runtime instrumentation and added initial example of "event loop lag" metrics.
I would refrain from adding more metrics in this PR to not block it for too long and add new metrics in subsequent PR.

The "event loop lag" metrics are inspired by https://github.com/marcbachmann/opentelemetry-node-metrics which is inspired by https://github.com/siimon/prom-client.

Comment on lines 22 to 33
/**
* Runtime instrumentation for Opentelemetry
*/
export class RuntimeInstrumentation extends InstrumentationBase {
constructor(_config: RuntimeInstrumentationConfig = {}) {
super("@opentelemetry/instrumentation-runtime", VERSION, _config);
}

init() {
createEventLoopLagMetrics(this.meter, this._config);
}
}
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 am not sure if this is the approach for "metric only" instrumentation. The host-metrics module is not using this approach[1].

[1] https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/packages/opentelemetry-host-metrics

@codecov
Copy link

codecov bot commented Nov 6, 2022

Codecov Report

Merging #1272 (5a4db76) into main (40515c3) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1272      +/-   ##
==========================================
+ Coverage   96.08%   96.20%   +0.12%     
==========================================
  Files          14       15       +1     
  Lines         893      923      +30     
  Branches      191      191              
==========================================
+ Hits          858      888      +30     
  Misses         35       35              
Impacted Files Coverage Δ
...trumentation-runtime/src/metrics/event-loop-lag.ts 100.00% <100.00%> (ø)

@mentos1386 mentos1386 force-pushed the feat-instrumentation-runtime branch from 1e3c802 to 5a4db76 Compare November 6, 2022 13:46
unit: "seconds",
})
.addCallback(async (observable) => {
const startTime = process.hrtime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the lowest version of Node is 14, process.hrtime.bigint() can be used instead.


histogram.enable();

createEventLoopLagMetrics(this.meter, histogram);
Copy link
Contributor

Choose a reason for hiding this comment

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

A meter can change while the application is running. The metrics need to be recreated on the new meter.
More context on the issue: open-telemetry/opentelemetry-js#3249

@mentos1386
Copy link
Contributor Author

@Ugzuzg thanks for the review! Im a bit busy these days, but i'll try to make some time and fix the mentioned issues.

Are there any comments on the overall structure that i should address?

@tomerfriedman
Copy link

tomerfriedman commented Jan 10, 2023

great! We really really need this! :) Is there an ETA as to when it'll be available?

@mentos1386

@stoberov
Copy link

stoberov commented Jan 19, 2023

+1

This feature will be greatly appreciated. <3

@blumamir
Copy link
Member

blumamir commented Feb 8, 2023

@mentos1386 any update on this PR? are you still interested in finishing it?

@github-actions
Copy link
Contributor

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Apr 10, 2023
@github-actions
Copy link
Contributor

This PR was closed because it has been stale for 14 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants