-
Notifications
You must be signed in to change notification settings - Fork 528
services/friendbot: Instrument friendbot to emit traces #5796
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Signed-off-by: Satyam Zode <[email protected]>
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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.
Pull Request Overview
This PR instruments the friendbot service to emit OpenTelemetry traces that will be sent to Grafana Alloy and forwarded to Grafana Tempo for distributed tracing capabilities and performance monitoring.
- Added a new
utils/tracer
package with OpenTelemetry configuration - Updated friendbot service to use context-aware tracing throughout the request lifecycle
- Added OpenTelemetry middleware to the HTTP router for automatic request tracing
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
utils/tracer/tracer.go | New utility package for OpenTelemetry tracer initialization and configuration |
services/friendbot/main.go | Updated to initialize tracing, add OpenTelemetry middleware, and use chi/v5 router |
services/friendbot/internal/friendbot_handler.go | Added tracing instrumentation to HTTP request handlers |
services/friendbot/internal/friendbot.go | Updated Pay method to accept context for tracing |
services/friendbot/internal/minion.go | Added context parameter and span instrumentation to minion operations |
services/friendbot/internal/minion_test.go | Updated test functions to pass context parameter |
services/friendbot/internal/friendbot_test.go | Updated test functions to pass context parameter |
services/friendbot/friendbot.cfg | Added otel_endpoint configuration |
go.mod | Updated dependencies to include OpenTelemetry packages and chi/v5 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
) | ||
|
||
if err != nil { | ||
return nil, errors.Wrap(err, "failed to create exporter") |
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.
The error message 'failed to create exporter' is inconsistent with the previous error message on line 50 which says 'Error while creating exporter'. Consider using consistent messaging and grammar.
return nil, errors.Wrap(err, "failed to create exporter") | |
return nil, errors.Wrap(err, "Error while creating resource") |
Copilot uses AI. Check for mistakes.
serviceVersion = "1.0.0" //TODO: Change version | ||
) | ||
|
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.
The TODO comment suggests this hardcoded version should be changed. Consider using build-time injection or reading from a version file to make version management more maintainable.
serviceVersion = "1.0.0" //TODO: Change version | |
) | |
) | |
// serviceVersion should be set at build time using -ldflags "-X main.serviceVersion=1.2.3" | |
var serviceVersion = "dev" |
Copilot uses AI. Check for mistakes.
@@ -63,6 +72,15 @@ func run(cmd *cobra.Command, args []string) { | |||
os.Exit(1) | |||
} | |||
|
|||
//Setup and intialize tracer |
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.
Spelling error: 'intialize' should be 'initialize'.
//Setup and intialize tracer | |
//Setup and initialize tracer |
Copilot uses AI. Check for mistakes.
address := r.Form.Get("addr") | ||
if address == "" { | ||
span.SetStatus(codes.Error, "missing destination account address") | ||
span.SetAttributes(attribute.String("error.type", "missing_parameter")) |
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.
This condition sets span status to error but doesn't return early, allowing execution to continue with an empty address which will likely cause issues downstream. The function should return an error when address is empty.
span.SetAttributes(attribute.String("error.type", "missing_parameter")) | |
span.SetAttributes(attribute.String("error.type", "missing_parameter")) | |
return "", problem.MakeInvalidFieldProblem("addr", nil) |
Copilot uses AI. Check for mistakes.
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
CHANGELOG.md
within the component folder structure. For example, if I changed horizon, then I updated (services/horizon/CHANGELOG.md. I add a new line item describing the change and reference to this PR. If I don't update a CHANGELOG, I acknowledge this PR's change may not be mentioned in future release notes.semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
services/friendbot: Instrument friendbot to emit traces. Traces will be sent to Grafana alloy which will send traces to Grafana tempo.
Why
Known limitations
This PR updates several dependencies. We need land this carefully.
Issue:
https://github.com/stellar/ops/issues/4005