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

feat: Add default HTTP log receiver #1330

Merged
merged 8 commits into from
Nov 10, 2023
Merged

Conversation

dpaasman00
Copy link
Contributor

Proposed Change

This PR adds a new receiver to the bindplane-agent which serves as a default HTTP log receiver. In actuality this was created for the sole purpose of ingesting logs from the Datadog-Agent. The receiver is made and described as a "default HTTP receiver" but the main focus of it is for this specific use case. As a result there are likely several limitations that prevent it from being a "default HTTP log receiver", but that is okay for the time being.

The correct and eventual solution for this use case and others like it is creating an HTTP input operator for Stanza and then making a receiver built around that, similar to the TCPLog receiver and Stanza TCP input operator. However that approach was not taken because we needed a solution for the Datadog-Agent use case quickly and the Stanza HTTP operator would take too long to realize.

Checklist
  • Changes are tested
  • CI has passed

@dpaasman00 dpaasman00 marked this pull request as ready for review November 8, 2023 14:18
@dpaasman00 dpaasman00 requested a review from a team as a code owner November 8, 2023 14:18
Copy link
Contributor

@schmikei schmikei left a comment

Choose a reason for hiding this comment

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

Looks mostly good I think, I think I still need to test but had one potential optimization that may or may not be worth doing

receiver/httpreceiver/logs.go Outdated Show resolved Hide resolved
receiver/httpreceiver/config.go Outdated Show resolved Hide resolved
receiver/httpreceiver/config.go Outdated Show resolved Hide resolved
receiver/httpreceiver/logs.go Outdated Show resolved Hide resolved
receiver/httpreceiver/README.md Show resolved Hide resolved
receiver/httpreceiver/config.go Outdated Show resolved Hide resolved
receiver/httpreceiver/logs.go Outdated Show resolved Hide resolved
receiver/httpreceiver/logs.go Outdated Show resolved Hide resolved
receiver/httpreceiver/logs.go Outdated Show resolved Hide resolved
receiver/httpreceiver/logs.go Show resolved Hide resolved
receiver/httpreceiver/logs.go Outdated Show resolved Hide resolved
Copy link
Contributor

@BinaryFissionGames BinaryFissionGames left a comment

Choose a reason for hiding this comment

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

Looking good, just one suggestion on the configuration

// Config defines the configuration for an HTTP receiver
type Config struct {
Path string `mapstructure:"path"`
ServerSettings *confighttp.HTTPServerSettings `mapstructure:"server"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Small recommendation here:

Suggested change
ServerSettings *confighttp.HTTPServerSettings `mapstructure:"server"`
ServerSettings *confighttp.HTTPServerSettings `mapstructure:",squash"`

This way we don't need the extra server key when specifying endpoint/tls. I think this is the usual pattern with this struct (e.g. https://github.com/open-telemetry/opentelemetry-collector/blob/2a9d870a57038cb228ef20e7620f18a93c92b9d3/receiver/otlpreceiver/config.go#L25)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yea perfect, didn't realize i could do that

Copy link
Contributor

@schmikei schmikei left a comment

Choose a reason for hiding this comment

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

Approving since I initially reviewed, I'll defer to the agent team on if/when to merge

@dpaasman00 dpaasman00 merged commit e1fa864 into release/v1.39.0 Nov 10, 2023
12 checks passed
@dpaasman00 dpaasman00 deleted the feat/http-receiver branch November 10, 2023 13:26
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.

4 participants