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 the otelcol syslog receiver #2263

Merged
merged 27 commits into from
Jan 3, 2025

Conversation

dehaansa
Copy link
Contributor

PR Description

While adding the otelcol.exporter.syslog component, I discovered that there was not a clean way to proxy syslog using the loki.source.syslog component as a source. For the purposes of syslog proxying, the otelcol.receiver.syslog component emits logs in the exact syntax expected by the exporter.

This does not add support for enhancing parsing with the numerous stanza operators available in upstream opentelemetry. Most or all of the use cases that would be supported by those can also be implemented through processors like the otelcol.processor.transform component, and the effort to add them in seemed significant for no known customer requirements at this time.

Which issue(s) this PR fixes

Related to #312

Notes to the Reviewer

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated
  • Config converters updated

@dehaansa dehaansa requested review from clayton-cornell and a team as code owners December 11, 2024 18:37
@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Dec 11, 2024
dehaansa and others added 4 commits December 11, 2024 14:59
…log.md

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
…log.md

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
dehaansa and others added 4 commits December 11, 2024 21:47
…log.md

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
The `location` argument specifies a Time Zone identifier. The available locations depend on the local IANA Time Zone database.
See [this wikipedia entry][tz-wiki] for a non-comprehensive list.

The `non_transparent_framing_trailer` argument must be one of `LF`, `NUL`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add some context around this? Like how can the default be nil? Even if its a string it should be "" but that seems to not be LF or NUL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's fair. It's a *string, so it's nil if not set. If it is set, then the character chosen is expected to terminate the message. If not set, it's expected that you're using octet counting or that the syslog frame contains only one message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@clayton-cornell I'd appreciate any feedback you have on this docs change as well as Matt's.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's in the docs makes sense to me. I don't know the specific nuances of a string data type and a value of nil though. Whats' there seems to make sense as I read it.

| Name | Type | Description | Default | Required |
|---------------------------------|----------|--------------------------------------------------------------------------------------------------------------|---------|----------|
| `listen_address` | `string` | The `<host:port>` address to listen to for syslog messages. | | yes |
| `max_log_size` | `int` | The maximum size of a log entry to read before failing. | `1MiB` | no |
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a string.

| Name | Type | Description | Default | Required |
|--------------------|------------|-----------------------------------------------------------------------------------------------------------|--------------|----------|
| `enabled` | `bool` | If true, the receiver will pause reading a file and attempt to resend the current batch of logs on error. | `false` | no |
| `initial_interval` | `duration` | The time to wait after first failure to retry. | `1 second` | no |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Durations should be in formatted to 1s or 5m

}

// Values taken from tcp input Build function
const defaultMaxLogSize = helper.ByteSize(tcp.DefaultMaxLogSize)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Rename to tcpDefaultMaxLogSize

Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

Looks good aside from a few doc changes.

dehaansa and others added 3 commits December 12, 2024 10:20
@dehaansa dehaansa requested a review from mattdurham December 12, 2024 15:59
Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

The updated description makes sense when I read it.

In reviewing again, I found a couple other minor points I missed in the first pass.

@clayton-cornell
Copy link
Contributor

Ooops double posted my review. I was logged out Git right when I submitted the review.

dehaansa and others added 2 commits December 12, 2024 13:47
…log.md

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
Copy link
Contributor

@clayton-cornell clayton-cornell left a comment

Choose a reason for hiding this comment

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

Approving for docs

Copy link
Collaborator

@mattdurham mattdurham left a comment

Choose a reason for hiding this comment

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

Lgtm, looks like a test is failing but will restart

@dehaansa dehaansa merged commit 89e17b2 into grafana:main Jan 3, 2025
15 of 16 checks passed
@dehaansa dehaansa deleted the feat/otelcol-syslog-receiver branch January 3, 2025 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Docs Squad label across all Grafana Labs repos
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants