-
Notifications
You must be signed in to change notification settings - Fork 150
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
LOG-6236: add docs about how to forwarding log to Syslog #2907
base: master
Are you sure you want to change the base?
Conversation
@vparfonov: This pull request references LOG-6236 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.8.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/hold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually thinking the table would be part of the apis/observability
{.foo.bar."baz/with/slashes"} | ||
|
||
. `appName`: is APP-NAME part of the syslog-msg header. | ||
`appName` needs to be specified if using `rfc5424`. The maximum length of the final values is truncated to 48 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a clarification here and similar in the remaining part of the doc. This is a correct statement but it reads to me like we expect a user to set this in our API. We do not need them to set it in the API because we are going to provide defaults. I might even consider removing the statement all-to-gether. cc @cahartma thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just add description that this is OPTIONAL and don't set if you are not sure, because i think in some cases appName
can be useful
|
||
. `appName`: is APP-NAME part of the syslog-msg header. | ||
`appName` needs to be specified if using `rfc5424`. The maximum length of the final values is truncated to 48 | ||
This supports template syntax to allow dynamic per-event values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could pull all of these statements for all of these fields into single statement. Identify which ones support templating, what is the correct syntax, how it will be treated, etc. This will remove the redundancy and repeated verbiage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
. `enrichment`: `None` or `KubernetesMinimal`. | ||
`None` add no additional enrichment to the record. | ||
`KubernetesMinimal` adds `namespace_name`, `pod_name`, and `collector_name` to the beginning of the message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should apply only to container logs. Did we "fix" vector to only apply these fields if the appropriate meta existed? I'm thinking of the case where we saw something like pod_name= coontainer_name=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to the Vector code, such situation possible if value no available, see: https://github.com/Clee2691/vector/blob/release-6.1/lib/codecs/src/encoding/format/syslog.rs#L335-L354
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
. `enrichment`: `None` or `KubernetesMinimal`. | ||
`None` add no additional enrichment to the record. | ||
`KubernetesMinimal` adds `namespace_name`, `pod_name`, and `collector_name` to the beginning of the message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be container_name not collector_name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
. `rfc`: Syslog RFC specification: `RFC3164` or `RFC5424` | ||
. `url`: an absolute URL, with a scheme. Valid schemes are: `http`, `https`, `tcp`, `tls`, `udp` and `udps` | ||
. `severity`: values are defined in https://tools.ietf.org/html/rfc5424#section-6.2.1 | ||
The value can be a decimal integer or one of these case-insensitive keywords: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true? Have we tried giving it a number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug | ||
|
||
. `facility`: values are defined in https://tools.ietf.org/html/rfc5424#section-6.2.1. | ||
The value can be a decimal integer. Facility keywords are not standardized, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. `payloadKey`: specifies record field to use as payload. This supports template syntax to allow dynamic per-event values. | ||
The `payloadKey` must be a single field path encased in single curly brackets `{}`. | ||
Field paths must only contain alphanumeric and underscores. Any field with other characters must be quoted. | ||
If left empty, Syslog will use the whole message as the payload key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should say:
... the whole message as the payload.
|
||
. `msgId`: is `MSGID` part of the syslog-msg header. This supports template syntax to allow dynamic per-event values. MsgId needs to be specified if using `rfc5424`. The maximum length of the final values is truncated to 32. This supports template syntax* to allow dynamic per-event values. | ||
|
||
. `enrichment`: `None` or `KubernetesMinimal`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add note this applies only to container logs. Consider working it as "application logs" and/or "infrastructure container" logs
|
||
Format: `+<PRI> VERSION TIMESTAMP HOSTNAME APP-NAME PROCID MSGID [STRUCTURED-DATA] MESSAGE+` | ||
|
||
Example: `+<PRI>1 2024-11-08T14:35:04.123Z ip-10-0-88-196.ec2.internal app-name 1234 ID47 [exampleSDID@32473 iut="3" eventSource="Application" eventID="1011"] This is a sample message+` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing structured data from the example. AFAIK we do not explicitly do anything to populate this or allow a user to control what goes into this field
Example: `+<34>Oct 11 22:14:15 mymachine su[1234]: 'su root' failed for lonvick on /dev/pts/+` | ||
|
||
|=== | ||
| | journal |application|infrastructure|note |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a little bit of conflicting statements here because of how we define log type. Infra is container or journal where application is only container. We should probably change app and infra to just be "container" and then add "audit" as another header since we process those. Also the header could be something like:
Infrastructure Journal, Infrastructure/Application Container, Audit
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcantrill, vparfonov The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
138ebc9
to
8d44619
Compare
Signed-off-by: Vitalii Parfonov <[email protected]>
8d44619
to
7b1129e
Compare
/test e2e-target |
/retest |
@vparfonov: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Description
Add document with description how to configure Log Forwarding to the Syslog receiver
/cc @Clee2691 @cahartma
/assign @jcantrill
/cherry-pick
Links