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

Add OTLP ingest #202

Merged
merged 29 commits into from
Jan 25, 2021
Merged

Add OTLP ingest #202

merged 29 commits into from
Jan 25, 2021

Conversation

MikeGoldsmith
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith commented Jan 7, 2021

Adds OTLP ingest support to refinery.

Initially this PR adds ingest support but does not add support for sending events to peer refinery processes or to the Honeycomb ingest API over gRPC and continues to use HTTP/JSON for that communication.

The primary use-case for adding OTLP here is for ingesting spans directly from OTel SDKs and Collector using OTLP over gRPC. This will allow users of OpenTelemetry to send data to refinery without having to use a custom span exporter and instead use the SDK provided OTLP exporter.

fix config reload test

only atempt to stop grpc server if not nil

don't double invoke config in config test
config/config.go Outdated Show resolved Hide resolved
route/route.go Outdated Show resolved Hide resolved
route/route.go Outdated Show resolved Hide resolved
Co-authored-by: asdvalenzuela <asdvalenzuela@honeycomb.io>
@vreynolds
Copy link
Contributor

@MikeGoldsmith I tried to review to the best of my knowledge :)

  • When I set this branch up locally, Refinery fails to start, because it attempts to bind a peer GRPC connection to the same GRPCListenAddr as main. I could be doing something wrong? Or we need to add a configuration for a peer GRPC address.
  • Need to add new config value to config_complete.toml (unless you plan on doing that in a separate PR)
  • config live reload is not something we intend to support, so I think you can omit those tests

Copy link
Contributor

@paulosman paulosman left a comment

Choose a reason for hiding this comment

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

Approving w/ some suggestions. Overall looks good!

route/route.go Outdated Show resolved Hide resolved
route/route.go Outdated Show resolved Hide resolved
route/route.go Show resolved Hide resolved
route/route.go Outdated Show resolved Hide resolved
route/route.go Outdated Show resolved Hide resolved
route/route_test.go Show resolved Hide resolved
Co-authored-by: Paul Osman <paul@honeycomb.io>
Copy link
Contributor

@vreynolds vreynolds left a comment

Choose a reason for hiding this comment

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

🏅

if len(grpcAddr) > 0 {
l, err := net.Listen("tcp", grpcAddr)
if err != nil {
r.iopLogger.Error().Logf("failed to listen to grpc addr: " + grpcAddr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to return if there's an error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should fail to start if a grpc address has been set and we're unable to bind to the port. We return if the HTTP server is unable to start. If something is reliant on gRPC running and the process doesn't fail to start, it could look like we're working but would reject gRPC traffic.

route/route.go Show resolved Hide resolved
@MikeGoldsmith MikeGoldsmith merged commit 3d85a7b into main Jan 25, 2021
@MikeGoldsmith MikeGoldsmith deleted the mike/otlp branch January 25, 2021 12:33
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.

None yet

4 participants