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 the otelarrow receiver scaffold #26519

Closed
wants to merge 9 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Sep 7, 2023

Description: Introduces scaffolding for the new otelarrow receiver.

Link to tracking Issue: #26491

Testing: No new tests, since this is just scaffold. Tests will arrive with the implementation.

Documentation: This README is copied (w/ some URLs updated) from the otel-arrow repository, which housed the component under development. Note this component is already functional, since the scaffold refers to the other repo's components (i.e., its Config and NewFactory).

@jmacd jmacd requested a review from a team as a code owner September 7, 2023 20:05
@jmacd jmacd requested a review from dashpole September 7, 2023 20:05
@jmacd jmacd assigned atoulme and unassigned Aneurysm9 Sep 7, 2023
@jmacd
Copy link
Contributor Author

jmacd commented Sep 7, 2023

Sorry, I realize you'll want this scaffold to use the fields generated from metadata.yaml. I'll re-open.

@jmacd jmacd closed this Sep 7, 2023
…mented; use metadata.Type and generated stability level
@jmacd
Copy link
Contributor Author

jmacd commented Sep 7, 2023

I replaced the use of the Config type alias and use of a NewFactory delegate, so now this PR includes the real Config and NewFactory. The config tests have been restored (with testdata, much of this of course stems from the core OTLP receiver component. Half of the factory test has been preserved here, changed to reflect the currently unimplemented status.

@jmacd jmacd reopened this Sep 7, 2023
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for breaking this PR up. See comments inline

receiver/otelarrowreceiver/metadata.yaml Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
type Protocols struct {
GRPC *configgrpc.GRPCServerSettings `mapstructure:"grpc"`
HTTP *httpServerSettings `mapstructure:"http"`
Arrow *ArrowSettings `mapstructure:"arrow"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a separate section for configuration? If the only setting is a memory limit, any reason not to include it at the top level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relates to open-telemetry/otel-arrow#43. This code is designed to drop-in where an OTLP receiver once stood, so leaving the Arrow settings in a separate section for future compatibility (was the idea).

receiver/otelarrowreceiver/config.go Show resolved Hide resolved
receiver/otelarrowreceiver/factory.go Outdated Show resolved Hide resolved
return err
}

// Note: since this is the OTel-Arrow exporter, not the core component,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is http only not a valid configuration for this receiver? if it is, then i would remove this comment and re-enable the check below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HTTP only means you've disabled OTel Arrow and you're identical to an OTLP receiver. I'm not sure why the user would want this. (Again, open-telemetry/otel-arrow#43 comes up.)

README states:

This receiver listens on the standard OTLP/gRPC port 4317 and serves standard OTLP over gRPC out of the box.

and later

To enable optional OTLP/HTTP support, the HTTP protocol must be explicitly listed. It will use port 4318 by default. The OTel Arrow protocol is not currently supported over HTTP.

if we added OTel Arrow support for HTTP streams, possibly, then it would make sense to apply the change you described, but I think we should separate the two transports into separate components.

jmacd and others added 3 commits September 7, 2023 16:03
Co-authored-by: Alex Boten <alex@boten.ca>
…try-collector-contrib into jmacd/receiver_scaffold
@jmacd
Copy link
Contributor Author

jmacd commented Sep 19, 2023

I have updated the README to declare this a multi-protocol receiver the way OTLP is a multi-protocol receiver.

This is a multi-protocol telemetry receiver.  Receives telemetry data
using the standard forms of [OTLP (gRPC, HTTP/proto, HTTP/json)](
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/otlp.md)
and [OTel-Arrow](https://github.com/open-telemetry/otel-arrow) (gRPC).

@jmacd
Copy link
Contributor Author

jmacd commented Sep 19, 2023

@jmacd
Copy link
Contributor Author

jmacd commented Sep 28, 2023

I will re-open this PR when we have a strategy to avoid duplication of the core OTLP exporter/receiver code.

@jmacd jmacd closed this Sep 28, 2023
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