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

Add support for logs in sdk-node #3826

Closed
pksunkara opened this issue May 20, 2023 · 10 comments · Fixed by #3969
Closed

Add support for logs in sdk-node #3826

pksunkara opened this issue May 20, 2023 · 10 comments · Fixed by #3969
Assignees

Comments

@pksunkara
Copy link

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

With the recent implementation of logs, the purpose of sdk-node (of being the only single import) is no longer effective.

Describe the solution you'd like

Add support for logs in sdk-node library.

Describe alternatives you've considered

Additional context

@pksunkara
Copy link
Author

@pichlermarc Can I please know whether this is in the plan or not? Is there any chat room where I can ask questions about this repo?

@pichlermarc pichlermarc added the up-for-grabs Good for taking. Extra help will be provided by maintainers label Jun 1, 2023
@pichlermarc
Copy link
Member

I think this should be done at some point, yes. The Logs SDK is still rather new, so no one has gotten around to updating the sdk-node package yet. I'll mark this as up-for-grabs in case someone has cycles to pick this up - we'd definitely appreciate PRs adding this. 🙂

To ask questions, you can open a "Discussion" issue here or the discussions forum on the repo. Alternatively, there's also the CNCF Slack (https://slack.cncf.io/).

@psk001
Copy link
Contributor

psk001 commented Jun 14, 2023

Hi @pichlermarc , as this issue is up for grabs , can I start working on it?

@pichlermarc
Copy link
Member

@psk001 sure, go ahead 🙂 - it's yours 🎉

@psk001
Copy link
Contributor

psk001 commented Jun 14, 2023

Great , I have set it up locally and running it . Could you please guide me a bit on how to proceed further ?

@pichlermarc
Copy link
Member

Great , I have set it up locally and running it . Could you please guide me a bit on how to proceed further ?

Sure. 🙂 First, you'll need to add @opentelemetry/api-logs and @opentelemetry/sdk-logs as dependencies in @opentelemetry/sdk-node (make sure to use the latest version 0.40.0 so that lerna links the package from the monorepo). You'd need to add a way to set a logRecordProcessor to NodeSDKConfiguration. Then in the SDKNode class, you can keep track of it and use that LogRecordProcessor to instantiate a LogRecordProvider in the SDKNode.start(). Then register it with the logs API (@opentelemetry/api-logs) and that should be it. 🙂 Metrics is already there and adding logs would be a similar approach, so you can look at how we instantiate metrics in @opentelemetry/sdk-node and use that as a way to guide you through the process. If you do that, you can ignore everything related to Views as that concept does not exist for logs.

Once that is done, you can add tests, and then you should be ready to open a PR. Once you open the PR, you'll have to sign the Contributor License Agreement (a bot will ask you for it), and then the approvers will review the PR. 🙂

@psk001
Copy link
Contributor

psk001 commented Jun 14, 2023

Got it, I'll proceed with it as suggested.

@psk001
Copy link
Contributor

psk001 commented Jun 21, 2023

Sure. slightly_smiling_face First, you'll need to add @opentelemetry/api-logs and @opentelemetry/sdk-logs as dependencies in @opentelemetry/sdk-node (make sure to use the latest version 0.40.0 so that lerna links the package from the monorepo). You'd need to add a way to set a logRecordProcessor to NodeSDKConfiguration

This is added.
I'm using metric implementation as reference to implement logs as you said.
Metric uses MetricReader as shown below

export type MeterProviderConfig = { reader?: MetricReader; views?: View[]; };

Is logger in sdk-logs the right equivalent for MetricReader in sdk-metrics ?

export type LoggerProviderConfig = { logger?: Logger; }

is it correct?

@pichlermarc
Copy link
Member

Sure. slightly_smiling_face First, you'll need to add @opentelemetry/api-logs and @opentelemetry/sdk-logs as dependencies in @opentelemetry/sdk-node (make sure to use the latest version 0.40.0 so that lerna links the package from the monorepo). You'd need to add a way to set a logRecordProcessor to NodeSDKConfiguration

This is added. I'm using metric implementation as reference to implement logs as you said. Metric uses MetricReader as shown below

export type MeterProviderConfig = { reader?: MetricReader; views?: View[]; };

Is logger in sdk-logs the right equivalent for MetricReader in sdk-metrics ?

export type LoggerProviderConfig = { logger?: Logger; }

is it correct?

It would be the LogRecordProcessor actually (and that one has an exporter attached to it 🙂)

export type LoggerProviderConfig = { logRecordProcessor?: LogRecordProcessor; }

@psk001
Copy link
Contributor

psk001 commented Jun 21, 2023

OK got it, I missed that part.

@pichlermarc pichlermarc removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants