-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[paymentservice] Adds otel logging support #1481
Conversation
There was a problem hiding this 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.
Okay, similar thing as with the accounting service. Any recommendations on what to do? |
@puckpuck I've just reformatted the require statements as you requested. Could you take a look? |
There was a problem hiding this 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
@puckpuck @austinlparker @julianocosta89 is there a library or a stable |
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. |
I answered on Slack as well, but for the record here: the OTel JS package |
@julianocosta89 I've done discussed with trentm I will use |
@trentm @julianocosta89 I have replaced pino with bunyan and done the neccessary instrumentation. |
@Kimbohlovette you can build this service locally and then start the demo locally. |
Okay I am on it |
@julianocosta89, I've updated the environment variables. I would like you to look at the instrumentation. |
@@ -1,59 +1,68 @@ | |||
// Copyright The OpenTelemetry Authors | |||
// SPDX-License-Identifier: Apache-2.0 | |||
const grpc = require('@grpc/grpc-js') |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay
We would like to have OTLP logs, if it works we will definitely get it merged. |
d4f39c0
to
762af5e
Compare
Okay |
I've made the requested changes |
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 |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Hello @pichlermarc, I have tried to use |
@trentm I tried bunyan but no lock too |
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 |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
hi guys, is there any news on that? I also want to enable logging in the payment service. |
This PR adds otel instrumentation of the pino library to the payment service