-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat(tracing): Initial setup for otel tracing #10
Conversation
Signed-off-by: Ritesh Rai <[email protected]>
Signed-off-by: Ritesh Rai <[email protected]>
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.
Tracing span implementation looks good 👍🏻 I had a couple nits and one blocker around the HTTP endpoint
main.go
Outdated
// Start HTTP server. | ||
srv := &http.Server{ | ||
Addr: ":8080", | ||
BaseContext: func(_ net.Listener) context.Context { return ctx }, | ||
ReadTimeout: time.Second, | ||
WriteTimeout: 10 * time.Second, | ||
Handler: newHTTPHandler(), | ||
} |
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.
I have some concerns about also using an HTTP server here for the health check, the provider does already have a health endpoint that's accessible over NATS. Is this health endpoint primarily used to demonstrate a trace or is it meant to be an indicator of provider health?
At the very least I think we should change this to a different port than 8080
since it's so common to use that for our HTTP endpoints for wasmCloud apps
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 purpose of this endpoint was to demonstrate that the trace is working when integrating via Jaeger . I am ok to change it . Do you think its better to remove this http endpoint ?
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.
I updated the port and simplified the tracing to replicate how it will be executed from the GET, PUT and other functions.
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.
I think it would be best to remove it and use the wasmCloud NATS health check endpoint for testing tracing if that's possible, that should be possible with the handleHealthCheck
option with the provider SDK
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.
Ok . I deleted the http health check endpoint. Have already verified that tracing with otel setup works here.
Signed-off-by: Ritesh Rai <[email protected]>
Signed-off-by: Ritesh Rai <[email protected]>
main.go
Outdated
// traceProvider := otel.GetTracerProvider() | ||
// tracer := traceProvider.Tracer("healthcheck") |
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.
If this code isn't needed, can we remove it?
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.
removed.
Signed-off-by: Ritesh Rai <[email protected]>
This PR will be used as a base for otel tracing in the provider.