-
Notifications
You must be signed in to change notification settings - Fork 9
LOG-5737: OpenShift Logging Conventions #6
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
Conversation
|
Sorry for accidentally pushing to the wrong fork. 😞 |
periklis
left a comment
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.
LGTM
cluster-logging.md
Outdated
|
|
||
| ### Log Entry Structure | ||
|
|
||
| All log streams include the following [logData](https://opentelemetry.io/docs/specs/otel/logs/data-model/#log-and-event-record-definition) fields |
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 makes more sense to call logRecords since its the key rather than logData.
Is there any value in an example snippet of the yaml 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.
A snippet from here, may be helpful:
https://github.com/open-telemetry/opentelemetry-proto/blob/main/examples/logs.json#L28
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 chose "logData", which should have been "log data" because that is the header text for the spec, though in general I find this page only useful when paired with the proto buff definition. It is not obvious, IMO, how to build the payload from this document.
cluster-logging.md
Outdated
| * `scope` for a scope attribute | ||
| * `log` for a log attribute | ||
|
|
||
|
|
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.
cluster-logging.md
Outdated
| | `body` | all | | | ||
| | `observedTimeUnixNano` | all | | | ||
| | `timeUnixNano` | all | | | ||
| | `severityNumber` | container, journal | | |
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.
Do we already send a "log level number"? If not, we might skip on this and continue just using the text representation. We can still add the field when someone needs it...
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 do not send the number in Viaq, all instance are translated from number to text and text has a pretty specific definition here which seems excessive.
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.
Settled on removing severityNumber
JoaoBraveCoding
left a comment
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.
lgtm
This PR:
Ref: https://issues.redhat.com/browse/LOG-5737
cc @
cahartma @pavolloffay @xperimental @periklis