[release-6.0] LOG-6756: Add timestamp to fix console #2965
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@cahartma: This pull request references LOG-6756 which is a valid jira issue. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cahartma 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 |
openshift-ci
bot
added
the
approved
|
@cahartma: 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. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
jcantrill
added
release/6.0
and removed
do-not-merge/hold
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "@timestamp": "2023-10-17T20:46:29.048949Z", | |||
| "timestamp": "2023-10-17T20:46:29.048949Z", | |||
There was a problem hiding this comment.
Why did @timestamp disappear for journal?
There was a problem hiding this comment.
Good catch, I can't recall off hand. Will investigate, as I suspect I had a reason for doing so.
| @@ -61,6 +63,7 @@ func NewReceiver(ns, name string) *Receiver { | |||
| "-server.grpc-max-recv-msg-size-bytes", "20971520", | |||
| "-distributor.ingestion-rate-limit-mb", "200", | |||
| "-distributor.ingestion-burst-size-mb", "200", | |||
| "-validation.discover-log-levels=false", | |||
There was a problem hiding this comment.
Why does this fail now and didn't before this submission?
There was a problem hiding this comment.
Updated version of loki is the cause. Robert is aware and confirmed this is necessary.
There was a problem hiding this comment.
Comes from disabling detected_level in the Loki Operator. https://issues.redhat.com/browse/LOG-6286
It's the same for service_name, but the flag does not work properly for some reason. I've added a note, and instead added the field as a quick fix to the tests to include service_name in the expected query.
| @@ -53,14 +53,14 @@ var _ = Describe("Log Format matcher tests", func() { | |||
| It("match same time value", func() { | |||
| timestamp := "2013-03-28T14:36:03.243000+00:00" | |||
| nanoTime, _ := time.Parse(time.RFC3339Nano, timestamp) | |||
| Expect(types.AllLog{ContainerLog: types.ContainerLog{ViaQCommon: types.ViaQCommon{Timestamp: nanoTime}}}). | |||
| To(FitLogFormatTemplate(types.AllLog{ContainerLog: types.ContainerLog{ViaQCommon: types.ViaQCommon{Timestamp: nanoTime}}})) | |||
| Expect(types.AllLog{ContainerLog: types.ContainerLog{ViaQCommon: types.ViaQCommon{TimestampLegacy: nanoTime}}}). | |||
There was a problem hiding this comment.
shouldnt we be testing for population of both values since we are sending both for backwards compatibility?
There was a problem hiding this comment.
The original idea was we wanted to remove the original field because we determined it has no value. As an alternative, we kept both to help ensure no regression issues. But, you are correct. I'll edit to ensure both are tested here.
Description
Manual backport for 6.0. This fixes the console and aligns
timestampwith the existing@timestampWe are still awaiting a Loki update to time-based sharding that will prevent any date concern as originally discussed in the ticket.We will want to wait until the Loki update is implemented before marking this as resolved.
/cc @Clee2691 @vparfonov @xperimental
/assign @jcantrill
Links