search

open-telemetry / opentelemetry-js

OpenTelemetry JavaScript Client
https://opentelemetry.io
Apache License 2.0
2.7k stars 781 forks source link

metrics, add support for auto collecting default recommended metrics #623

Closed shaz1v closed 2 years ago

shaz1v commented 4 years ago

Is your feature request related to a problem? Please describe.

There are a lot of default metrics which are recommended and should be collected and exported in every node project running, implementing the manual collection of each one of them in every project could be solved by adding support in the lib, and by that engaging metrics lib as a lot more noob friendly.

Describe the solution you'd like

Add support for auto collecting default recommended metrics, sort of like all the plugins for the tracing lib, add a default metrics plugin, for the metrics lib.

Describe alternatives you've considered

Collecting all those metrics manually, (which is what I'm doing currently), although it would be nice (and super noob friendly) to add support for auto collecting those metrics.

Additional context

Prometheus has a list of recommended metrics to export, https://prometheus.io/docs/instrumenting/writing_clientlibs/#standard-and-runtime-collectors

Plus some node-specific ones from the prom-client library https://github.com/siimon/prom-client/tree/29b6d86c3c8246c8e7f14fcc39a16682e2248934/lib/metrics

Thanks!

mayurkale22 commented 4 years ago

+1

But I think we should wait until we have implemented full metrics SDK with aggregation support (#402 and #452). Also, I was thinking to create a dedicated package for default recommended metrics (one option is @opentelemetry/system-metrics)

bhupesh-sf commented 3 years ago

+1

Something like this will be great Swagger-stats

dyladan commented 3 years ago

The metrics sdk is still heavily underspecified and our implementation is an incomplete version of that incomplete specification. In my opinion it would be a mistake to implement too much on top of the current implementation.

bhupesh-sf commented 3 years ago

Any update here for good metrics support?

@dyladan

vmarchaud commented 3 years ago

@bhupesh-sf There is the host-metrics package available and an open PR here for nodejs metrics.

As the specification is still moving (and i believe the current focus of the specification group), the comment of @dyladan still stand, we should avoid implementing too much on top of a moving API.

bhupesh-sf commented 3 years ago

@vmarchaud Its great to see these packages. We are really moving towards what is really desired.

As I mentioned earlier a package https://github.com/slanatech/swagger-stats gives lots of great metrics enough for our almost all use cases. Right now we are using this for our project but would really like to have something like that available within opentelemetry.

Keep us posted :+1:

marcbachmann commented 3 years ago

I just did a small POC in https://github.com/marcbachmann/opentelemetry-node-metrics by extracting some code from https://github.com/siimon/prom-client

marcbachmann commented 3 years ago

The module mentioned above got migrated to bound instruments and is working properly. I've published it to npm as opentelemetry-node-metrics. I hope this can ease the adoption of @opentelemetry/metrics as long as the other PRs aren't merged.

legendecas commented 2 years ago

Closing as https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/packages/opentelemetry-host-metrics is published.

Luckz commented 2 years ago

Would argue merely having "host metrics" does not "complete" anything like "default recommended (application) metrics". Obviously goes for #955 too, which for example specifically mentions "http requests" and "like Spring Boot Actuator". #1330 also asks for "http request count".

legendecas commented 2 years ago

@Luckz HTTP instrumentation metrics are being worked in PR https://github.com/open-telemetry/opentelemetry-js/pull/3149.

Luckz commented 2 years ago

Yes, I tried to run commit fdd6f58 too but didn't manage to make it work.

However, that is HTTP as in Node's http package for HTTP/HTTPS, and thus also not necessarily what those issues were asking for.

legendecas commented 2 years ago

However, that is HTTP as in Node's http package for HTTP/HTTPS, and thus also not necessarily what those issues were asking for.

@Luckz I'm not sure I fully understand your point clearly. We are working towards adding specified metric signals in the instrumentation packages. Requesting new metric signals is always welcome.

I don't think we are going to include these signals in the @opentelemetry/sdk-metrics, same as not generating trace signals in @opentelemetry/sdk-traces-base. However, we are bundling several commonly used instrumentation in @opentelemetry/sdk-node, so that common signals are generated when using @opentelemetry/sdk-node when little configuration.