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

[paymentservice] Adds otel logging support #1481

Conversation

Kimbohlovette
Copy link

This PR adds otel instrumentation of the pino library to the payment service

@Kimbohlovette Kimbohlovette requested a review from a team as a code owner March 24, 2024 19:25
@Kimbohlovette Kimbohlovette changed the title Adds otel logging support to payment service [paymentservice] Adds otel logging support Mar 24, 2024
Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

Hello @Kimbohlovette,

I've tested this PR and couldn't see any logs being exported via OTLP.

I do see trace data being added to the logs, but the logs are not being sent to the collector.

@Kimbohlovette
Copy link
Author

Okay, similar thing as with the accounting service. Any recommendations on what to do?
@puckpuck

src/paymentservice/opentelemetry.js Outdated Show resolved Hide resolved
@Kimbohlovette
Copy link
Author

@puckpuck I've just reformatted the require statements as you requested. Could you take a look?

Copy link
Member

@austinlparker austinlparker left a comment

Choose a reason for hiding this comment

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

See comment on #1477

@Kimbohlovette
Copy link
Author

@puckpuck @austinlparker @julianocosta89 is there a library or a stable exporter for javascript that can be used to export logs to opentelemetry?

@julianocosta89 julianocosta89 marked this pull request as draft April 3, 2024 11:10
@julianocosta89
Copy link
Member

I spoke with @Kimbohlovette today and asked him to reach out to the otel-js team and check if the OTel logs are actually usable.
Whenever he gets a reply, he will either adjust this PR to properly export the logs, or close it as not implemented yet.

@trentm
Copy link

trentm commented Apr 4, 2024

I answered on Slack as well, but for the record here: the OTel JS package @opentelemetry/instrumentation-pino does not yet support the logs bridge API for sending log records via OTLP. I intend to work on that, but haven't gotten to it yet. (FWIW, the other instrumentation JS logging framework instrumentations -- Bunyan and Winston -- have added logs bridge support recently.)

@Kimbohlovette
Copy link
Author

@julianocosta89 I've done discussed with trentm I will use bunyan to implement this. So work is in progress

@Kimbohlovette
Copy link
Author

@trentm @julianocosta89 I have replaced pino with bunyan and done the neccessary instrumentation.
I would like you to test and check if logs and coming to otel correctly. And I would also like to ask if checking these logs in opentelemetry is something that I can do locally before I push

@Kimbohlovette Kimbohlovette marked this pull request as ready for review April 5, 2024 14:04
@julianocosta89
Copy link
Member

@Kimbohlovette you can build this service locally and then start the demo locally.
Once everything is up and running, you can navigate to grafana:
localhost:8080/grafana and check if the logs are arriving in the demo chart.

@Kimbohlovette
Copy link
Author

@Kimbohlovette you can build this service locally and then start the demo locally. Once everything is up and running, you can navigate to grafana: localhost:8080/grafana and check if the logs are arriving in the demo chart.

Okay I am on it

@github-actions github-actions bot added the helm-update-required Requires an update to the Helm chart when released label Apr 10, 2024
@Kimbohlovette
Copy link
Author

@julianocosta89, I've updated the environment variables. I would like you to look at the instrumentation.
Aside from that I looked at the log api which is currently in experimental and it looks like something that can work fine if we we want to keep things simple

.env Outdated Show resolved Hide resolved
@@ -1,59 +1,68 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0
const grpc = require('@grpc/grpc-js')
Copy link
Member

Choose a reason for hiding this comment

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

@Kimbohlovette could you keep the same indentation we had before?

It would make our life as reviewers easier

Copy link
Author

Choose a reason for hiding this comment

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

okay

Copy link
Author

Choose a reason for hiding this comment

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

okay

@julianocosta89
Copy link
Member

@julianocosta89, I've updated the environment variables. I would like you to look at the instrumentation. Aside from that I looked at the log api which is currently in experimental and it looks like something that can work fine if we we want to keep things simple

We would like to have OTLP logs, if it works we will definitely get it merged.

@Kimbohlovette Kimbohlovette force-pushed the Add-otel-log-support-to-payment-service branch from d4f39c0 to 762af5e Compare April 10, 2024 13:30
@Kimbohlovette
Copy link
Author

@julianocosta89, I've updated the environment variables. I would like you to look at the instrumentation. Aside from that I looked at the log api which is currently in experimental and it looks like something that can work fine if we we want to keep things simple

We would like to have OTLP logs, if it works we will definitely get it merged.

Okay

@Kimbohlovette
Copy link
Author

I've made the requested changes

@puckpuck
Copy link
Contributor

Please update your IDE settings to not modify the indentation and line wrap of code files. This will make reviewing the PR much easier.

I ran this branch, and did not see the traces or logs being emitted from the payment service. With a quicker look I noticed you added console exporters for both. I don't think you need any exporter, since the default should be OTLP, which is configured using environment variables.

@Kimbohlovette
Copy link
Author

Kimbohlovette commented Apr 11, 2024

Please update your IDE settings to not modify the indentation and line wrap of code files. This will make reviewing the PR much easier.

I ran this branch, and did not see the traces or logs being emitted from the payment service. With a quicker look I noticed you added console exporters for both. I don't think you need any exporter, since the default should be OTLP, which is configured using environment variables.

Alright, I've been finding where in vscode I can disable the automatic formatting on vscode for some time now

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 19, 2024
@Kimbohlovette
Copy link
Author

Hello @pichlermarc, I have tried to use @opentelemetry/sdk-logs in this service to send logs to open telemetry, Unfortunately I can not see any logs emitted on grafana.
Please can you look at my code to see what is not right?

@Kimbohlovette
Copy link
Author

@trentm I tried bunyan but no lock too

@github-actions github-actions bot removed the Stale label Apr 22, 2024
@pichlermarc
Copy link
Member

Hello @pichlermarc, I have tried to use @opentelemetry/sdk-logs in this service to send logs to open telemetry, Unfortunately I can not see any logs emitted on grafana. Please can you look at my code to see what is not right?

Looking at this PR's diff, it looks like there's no exporter configured other than the console log exporter. Are you using a different set of changes to export to grafana/loki? 🤔

@Kimbohlovette
Copy link
Author

Kimbohlovette commented Apr 22, 2024

Hello @pichlermarc, I have tried to use @opentelemetry/sdk-logs in this service to send logs to open telemetry, Unfortunately I can not see any logs emitted on grafana. Please can you look at my code to see what is not right?

Looking at this PR's diff, it looks like there's no exporter configured other than the console log exporter. Are you using a different set of changes to export to grafana/loki? 🤔

I don't have full understanding on how the logs gets to grafana dashboard but what I understand is that in this exercise open telemetry has already been configured to send logs to grafana (which is one of the servies in this app serving on 8080/grafana).

So the goal of this issue is to get the each service to send logs to open telemetry and then it will show on grafana under the current service

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Apr 30, 2024
Copy link

github-actions bot commented May 7, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
helm-update-required Requires an update to the Helm chart when released Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants