-
Notifications
You must be signed in to change notification settings - Fork 810
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
[sdk-node] automatically configure metrics exporter based on enviornment variables #4551
Comments
@pichlermarc I would like to work on this. I have used this package in the past. |
@Prashansa-K thanks for picking this up. It's yours 🙂 |
@pichlermarc
|
If the env var is set, only create a single
That's correct. 🙂 The specification reads:
so that's applicable to all exporter selection env vars. |
@pichlermarc is this behavior consistent across python,java, go etc? |
@pichlermarc would it be possible to have some answers on the above question? I am trying to understand if this by design for all nodejs layer.sdk or all other languages too? |
I'm not sure what the question is here - the feature is not implemented yet. As with all specification features, the behavior aims to be aligned across all languages. |
@pichlermarc what I mean to understand is whether python, go, java export metrics by deafult or do they not just like in nodejs. same goes for logs. |
@pichlermarc if you could please help me get answer on my above question? |
Hi there @pichlermarc If this choice was made by design, we would love to understand the reasoning behind this choice. To expedite progress, we would be happy to claim and/or contribute to this feature if needed. Please let us know @Prashansa-K |
I don't know the exact state of all the other language SDKs autoconfigure packages. I know that the specification does not prescribe a default. Posting research here would be very welcome 🙂
It was a conscious decision. As there was no way to set up the metrics exporter, automatically setting up the MeterProvider for it would've been more annoying than helpful until this feature here and #4655 are implemented. I updated the issue description to reflect that. When implementing this feature do NOT set up a MeterProvider if the OTEL_METRICS_EXPORTER env var is not set. We can revisit that decision when #4655 is implemented. |
Thanks @pichlermarc for the clarification. As we dig more, shall share the findings here. |
@Prashansa-K please help us understand how far the PR is. We have needs in this area and are happy to pair up, if that can expedite your contribution. |
@pichlermarc Is there anything we can do to expedite this? |
@bhaskarbanerjee I don't have any way to reach out to @Prashansa-K, unfortunately. Looks like there's no response to the above comment and the issue was assigned a little over a month ago. |
Thanks for prompt response @pichlermarc we will get on top of this. cc @silpamittapalli |
hi, what's the status on this? |
@haythemtrabelsi117 we got delayed but are now actively working on this. Please expect a PR by end of this month. |
@pichlermarc @dyladan |
@pichlermarc can you please assign this ticket to me? |
@bhaskarbanerjee please don't access the core module anymore to get the environment config, we're moving away from that. You can directly use |
Thanks for that info @pichlermarc , will follow that. |
@pichlermarc I am in process of drafting this PR. I see there is a function for configuring the log provider from env. The simplest way to solve this would be to add a similar function for configuring the metric provider and then write tests on top of this. Are you good with approach or do you have another suggestion?
And then add the below lines to end of https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/opentelemetry-sdk-node/src/sdk.ts#L212
|
@pichlermarc one more testing question. Apart from Unit testing, what other testing is encouraged and is there a designated process to do so? |
@pichlermarc any thoughts on the above? |
@bhaskarbanerjee yes that sounds good - some suggestion for the code above: you may want to immediately pair the exporter with a
There's only "unit"-tests in |
🆒 Thanks @pichlermarc My PR should be therein a few days. I will try my best with the unit tests.
|
@bhaskarbanerjee please read https://github.com/open-telemetry/opentelemetry-js/blob/be1737fc46c1c0fbe75f4d30ce5a196ebeee4c09/CONTRIBUTING.md#development-quick-start and follow the steps there - the steps that are listed there are necessary to setup auto-generated files that are required to make tests pass. After doing that once you can just run |
Thanks @pichlermarc I will check this out |
Currently, we don't auto-configure a
MetricReader
/exporter combination when using the@opentelemetry/sdk-node
package.Goal of this issue is to implement exporter selection for metrics based on this specification:
For this issue to be considered done we need to implement the following behavior:
If no metric reader is configured by the user:
OTEL_METRICS_EXPORTER
environment variable to determine an exporter and add it to theMeterProvider
that's created byNodeSDK
PeriodicExportingMetricReader
if it is aPushMetricExporter
MeterProvider
ifOTEL_METRICS_EXPORTER
is unset, any current code-only configuration should continue to work as it does now. If ametricReader
is already provided by the user, do NOT override or add an additional reader based on the contents ofOTEL_METRICS_EXPORTER
OTEL_EXPORTER_OTLP_METRICS_PROTOCOL
to determine the OTLP exporter to use (http/json, http/protobuf, grpc)OTEL_EXPORTER_OTLP_PROTOCOL
env var as a fallback to the above to determine the OTLP exporter to use (http/json, http/protobuf, grpc)The text was updated successfully, but these errors were encountered: