-
Notifications
You must be signed in to change notification settings - Fork 0
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
Initial import of lightstepreceiver #16
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.
- Running
gofmt -w -s .
andgoimports -w .
in the receiver directory will correct some formatting. Running those commands with path set to./..
could change generated files and that's generally discouraged. - The substantial parts of missing coverage are in
trace_receiver.go
, so adding something to the tests in there would be great even though those look like well known methods. Some coverage there could be especially good for running tests with-race
.
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.
Running gofmt -w -s . and goimports -w . in the receiver directory will correct some formatting. Running those commands with path set to ./.. could change generated files and that's generally discouraged.
The substantial parts of missing coverage are in trace_receiver.go, so adding something to the tests in there would be great even though those look like well known methods. I think it's a worthwhile enhancement even if you're going for a lift and shift port. Afterward you can run the tests with go test ./.. -race
and use goleak
to verify the code doesn't leak goroutines. I checked this out to be sure although I think it would be covered with lifecycle tests.. if this were in OTel repos.
We already test our translation layer extensively but want to do a full operation test as well.
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.
LGTM
Thanks for doing this, Carlos.
…t metadata.yaml) and fix some tests
By the way, I've updated some dependencies and fixed one of the leak-tests. |
Also having re-run a more-recent |
Merging this port of Lightstep receiver based on unit tests and manual reviews. |
Initial import of our lightstepreceiver, which will allow us to ingest data from legacy tracers. Overall:
localhost:443
. Maybe reconsider?