Skip to content
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

Merged
merged 11 commits into from
Jun 10, 2024
Merged

Initial import of lightstepreceiver #16

merged 11 commits into from
Jun 10, 2024

Conversation

carlosalberto
Copy link
Contributor

Initial import of our lightstepreceiver, which will allow us to ingest data from legacy tracers. Overall:

  • Defaults to sending data to localhost:443. Maybe reconsider?
  • Support for http/proto, with upcoming support for grpc (and maybe thrift).
  • No initial OBS report for now (i.e. no internal metrics reporting traces count and related)
  • Semantic conventions mapping is only done for errors, i.e. semconv is kept from OpenTracing.
  • Other limitations or potential improvements are listed in DESIGN.md

@carlosalberto carlosalberto marked this pull request as ready for review May 15, 2024 14:01
@carlosalberto carlosalberto requested a review from a team as a code owner May 15, 2024 14:01
Copy link
Contributor

@nslaughter nslaughter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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.
  2. 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.

@nslaughter nslaughter self-requested a review May 27, 2024 19:06
Copy link
Contributor

@nslaughter nslaughter left a 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.
@nslaughter nslaughter self-requested a review June 4, 2024 21:21
Copy link
Contributor

@nslaughter nslaughter left a 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.

@jmacd
Copy link
Member

jmacd commented Jun 10, 2024

By the way, I've updated some dependencies and fixed one of the leak-tests.
There is now a proto/ directory containing the lightstep protos, the google protos we depend on, and a script to re-run protoc.

@jmacd
Copy link
Member

jmacd commented Jun 10, 2024

Also having re-run a more-recent mdatagen on this component, there are new lifecycle tests and somewhat better coverage (now around 80%).

@nslaughter nslaughter merged commit ad0f84a into main Jun 10, 2024
1 of 2 checks passed
@nslaughter nslaughter deleted the add-lightstepreceiver branch June 10, 2024 18:49
@nslaughter
Copy link
Contributor

nslaughter commented Jun 10, 2024

Merging this port of Lightstep receiver based on unit tests and manual reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants