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

"Invalid log format" error in e2e tests with logstream #4777

Closed
abayer opened this issue Apr 19, 2022 · 4 comments · Fixed by #5159
Closed

"Invalid log format" error in e2e tests with logstream #4777

abayer opened this issue Apr 19, 2022 · 4 comments · Fixed by #5159
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@abayer
Copy link
Contributor

abayer commented Apr 19, 2022

When running e2e tests, logstream barfs when trying to take the JSON log entries fed to it and turn them into more readable lines, like this:

    stream.go:238: Invalid log format for pod tekton-pipelines-controller-f7c657f7-fsqsq: {"level":"info","ts":"2022-04-19T13:58:41.025Z","logger":"tekton-pipelines-controller","caller":"controller/controller.go:550","msg":"Reconcile succeeded","commit":"e055233","knative.dev/controller":"github.com.tektoncd.pipeline.pkg.reconciler.taskrun.Reconciler","knative.dev/kind":"tekton.dev.TaskRun","knative.dev/traceid":"d5fc50a5-db68-44f2-89cf-e726a9694151","knative.dev/key":"arendelle-87jkt/workspace-in-sidecar-hiyzzusj","duration":0.000339983}

This is because logstream changed the keys it's expecting to fit a knative/pkg change, with the result that logstream is expecting severity, not level, timestamp, not ts, and message, not msg. Our config-logging still uses the "old" keys.

The question, as I see it, is whether we should update config/config-logging.yaml to use the same keys as Knative and logstream, or if we should specifically hack the e2e test setup to use those keys instead.

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 19, 2022
@lbernick
Copy link
Member

I think this is worth doing in the config as well as in tests since it helps cut down on log spam when debugging.

@tekton-robot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 18, 2022
@abayer
Copy link
Contributor Author

abayer commented Jul 18, 2022

/lifecycle frozen

@tekton-robot tekton-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 18, 2022
@imjasonh
Copy link
Member

The question, as I see it, is whether we should update config/config-logging.yaml to use the same keys as Knative and logstream, or if we should specifically hack the e2e test setup to use those keys instead.

Is there a downside to updating our config-logging.yaml to match logstream's expectations? Installations and their monitoring that had previously looked for msg in logs would need to go looking in message instead. Do we know how likely/prevalent that expectation is?

In any case, #5159 seems to be one way to fix this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants