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
feature(backend): Adding support for HTTP OTLP server #2412
feature(backend): Adding support for HTTP OTLP server #2412
Conversation
examples/quick-start-jaeger-nodejs/tracetest/collector.config.yaml
Outdated
Show resolved
Hide resolved
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.
Some changes are needed, but overall, very good.
examples/quick-start-jaeger-nodejs/tracetest/collector.config.yaml
Outdated
Show resolved
Hide resolved
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 is great work oscar! my main concern is that the HTTP server is not following the same patterns as the main http service, which leads to a lot of code that can be replaced by libraries, and having 2 ways of doing the same thing
server/otlp/exporter.go
Outdated
db model.Repository | ||
|
||
gServer *grpc.Server | ||
type IExporter interface { |
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.
We usually don't prefix interfaces with I
in go. Also it looks like this interface is not really required. You can just reference the actual struct. And given that this is only used internally within this package, you can even unexport it (a.k.a lowercasing the first letter)
server/otlp/exporter.go
Outdated
addr: addr, | ||
db: db, | ||
} | ||
type Exporter struct { |
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.
It's a bit weird that an Exporter
only exposes an Ingest
method. I would rename either the struct or the method to look a bit more consistent
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.
Again, you can unexport this struct, since it's only directly referenced within the package
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 is looking really awesome. My only remaining question is about what I think is a duplicated gzip compression handler in the http server. Am I reading that wrong?
|
||
func (s *httpServer) Start() error { | ||
r := mux.NewRouter() | ||
r.HandleFunc("/v1/traces", s.Export).Methods("POST") |
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.
niceee
* creating http otlp server * cleanup changes * cleanup changes * fixing example * enabling JSON request body for the OTLP HTTP endpoint * PR comments * PR comments * test * manually installing dependencies
This PR enables the tracetest server to spin off an OTLP http port that listens to incoming spans
Changes
Fixes
Checklist
https://www.loom.com/share/e05c8a794a3c47dcb660121f23ca58ee