-
Notifications
You must be signed in to change notification settings - Fork 12
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
Migrate to OTEL libraries #2
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.
This PR unnecessarily large. Please split into independent pieces, eg
- Introduce go.mod
- Add OTEL SDK
and I don't see a need to move the files into pkg, why is that needed?
@yurishkuro yeah you're right. I planned to do a first PR for the god modules, but got some errors on the Jaeger packages and then the PTSD of working with go vendor kicked in D: so I thought, if I use the latest packages from OTEL it must surely work with go modules. I'll split it. The moving into |
cmd/pkg convention makes more sense for monorepo-style projects like Jaeger backend that produces a lot of different binaries. In this repo it provides no benefits. |
Thanks for go.mod change btw, I even forgot that this repo is still on |
Since the Jaeger SDK is deprecated use instead the OTEL SDK with the Jaeger exporter.
b132c1b
to
5d328c2
Compare
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.
Awesome!
Selfish ask - maybe you're interested in jaegertracing/jaeger#3380 ? :-)
"go.opentelemetry.io/otel/sdk/resource" | ||
tracesdk "go.opentelemetry.io/otel/sdk/trace" | ||
semconv "go.opentelemetry.io/otel/semconv/v1.12.0" | ||
"go.opentelemetry.io/otel/trace" | ||
) | ||
|
||
// JaegerCollectorURL defines the address of Jaeger collector to submit spans. | ||
var JaegerCollectorURL = "http://localhost:14268/api/traces" |
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.
Another option is to start using OTLP as exporter instead of Jaeger format, because Jaeger backend can already accept it. Or make it an option to switch between jaeger
and otlp
. But not in this PR.
Since the Jaeger SDK is deprecated use instead the OTEL SDK with the
Jaeger exporter.