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.

Sign up for GitHub

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

Jump to bottom

Bug 1966717: include full timestamps in the logs #442

Merged
merged 1 commit into from
Jun 3, 2021
Merged

Bug 1966717: include full timestamps in the logs #442

merged 1 commit into from
Jun 3, 2021

Conversation

Serhii1011010
Copy link
Contributor

Categories

  • Bugfix
  • Enhancement
  • Backporting
  • Others (CI, Infrastructure, Documentation)

Sample Archive

  • path/to/sample_data.json

Documentation

  • *.log files now have full timestamps

Unit Tests

No changes

Privacy

Changelog

Breaking Changes

Yes, the new format of timestamps in the logs

References

https://issues.redhat.com/browse/CCXDEV-4897
https://bugzilla.redhat.com/show_bug.cgi?id=???
https://access.redhat.com/solutions/???

@openshift-ci openshift-ci bot requested review from natiiix and psimovec June 1, 2021 18:07
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2021
@Serhii1011010 Serhii1011010 changed the title include full timestamps in the logs Bug 1966717: include full timestamps in the logs Jun 1, 2021
@openshift-ci openshift-ci bot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Jun 1, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 1, 2021

@Sergey1011010: This pull request references Bugzilla bug 1966717, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.8.0) matches configured target release for branch (4.8.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @psimovec

In response to this:

Bug 1966717: include full timestamps in the logs

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/test-infra repository.

@openshift-ci openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jun 1, 2021
tremes
tremes reviewed Jun 2, 2021
View reviewed changes
pkg/gatherers/clusterconfig/gather_logs.go
@@ -129,6 +130,10 @@ func filterLogs(

for scanner.Scan() {
line := scanner.Text()
if messagesToSearch == nil {
result += line + "\n"
}
Copy link
Contributor

@tremes tremes Jun 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This opens the new functionality for this function. It would be nice to unify it with https://github.com/openshift/insights-operator/blob/master/pkg/gatherers/clusterconfig/operators_pods_and_events.go#L254 one day....
Just saying..... otherwise LGTM, but we need to check with @jholecek-rh about potential impact on the rules.

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, I started refactoring to have only one place to gather logs, but then decided it doesn't worth the efforts right now.

@jholecek-rh
Copy link
Contributor

@Sergey1011010 Is it possible to tell the timestamp format from insights-operator/gathers.json? I'm looking for something for rules to easily detect if they can expect full timestamps or the original ones. It would be really helpful to distinguish why a rule is not hitting:

  1. It didn't have the required data (the full timestamp).
  2. It didn't find any log messages of interest (a regex didn't match any line).

I'm also wondering how to reflect this breaking change in the documentation. We will make sure that all existing rules will support both the old and the new format. However, we need to make sure that even new rules will do so.

@Serhii1011010
Copy link
Contributor Author

@jholecek-rh after merging this PR all the logs in the archives will have the new format. So, only the old archives are gonna stick to the old format. It's possible to use a simple regex to detect the format.

@tremes
Copy link
Contributor

tremes commented Jun 3, 2021

Just few changes
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 3, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Sergey1011010, tremes

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:
  • OWNERS [Sergey1011010,tremes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 110331d into openshift:master Jun 3, 2021
@openshift-ci
Copy link

openshift-ci bot commented Jun 3, 2021

@Sergey1011010: All pull requests linked via external trackers have merged:

Bugzilla bug 1966717 has been moved to the MODIFIED state.

In response to this:

Bug 1966717: include full timestamps in the logs

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/test-infra repository.

@jholecek-rh
Copy link
Contributor

@Sergey1011010 Let's be clear. "Only old archives" means all archives we've received and all archives we will have received until clusters upgrade to 4.8.something.That's a lot of archives/clusters. 🙂

Timestamps in log files are an archive-wide "configuration". To distinguish the two cases that I described, we need an Insights component that would serve as an archive-wide indicator of the log file structure.

Option 1:

  • We know the structure when writing the archive.
  • We pass the information in the archive.
  • The component picks it up.

Clean, robust and easy.

Option 2:

  • We know the structure when writing the archive.
  • We don't pass it on in the archive.
  • The component will
    • Depend on all logs in the archive because logs are collected conditionally and we don't know which logs are there
    • Need to handle the case that there are no log files at all
    • Base the decision on an (undocumented?) assumption that the first line of any log file determines the structure of all log files

Workable, not an easy way to consume the data, not exactly clean, possibly less robust.

This is my last try. I will not push further. 🙂

@jholecek-rh
Copy link
Contributor

@Sergey1011010 How about the documentation? Any ideas how to make consumers of the data aware that we are receiving archives with two different log file structures? Will we expect them to "just know"(tm) and wait for the bright distant future where all (most) clusters will have updated? 😇

@Serhii1011010
Copy link
Contributor Author

@jholecek-rh I think it should be pretty easy to make a regex inside the function processing log file that would check which timestamp format it is. We can add some flag somewhere in metadata, but not sure if we want that. @tremes WDYT?

@Serhii1011010
Copy link
Contributor Author

Random thought: we could also include the list of files produces by the function (if any), but I'm not sure if I like this idea, it could simplify processing.

@tremes
Copy link
Contributor

tremes commented Jun 4, 2021

Adding some flag to the metadata seems a bit redundant IMO. Couldn't the rule processing just distinguish the cluster versions and act accordingly? Because this update is/will be for >4.8.0.

@tremes
Copy link
Contributor

tremes commented Jun 4, 2021

Wrt documentation - we should have updated the sample archive at least right @Sergey1011010 ? I just realized.....

@jholecek-rh
Copy link
Contributor

OK, deciding based on the version seems OK. Is it going to be >4.8.0 (i.e. >=4.8.1) or >=4.8.0?

@Serhii1011010
Copy link
Contributor Author

@tremes yup, forgot about it, there it is #447

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tremes tremes tremes left review comments

@natiiix natiiix Awaiting requested review from natiiix

@psimovec psimovec Awaiting requested review from psimovec

Assignees

@tremes tremes

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Serhii1011010 @jholecek-rh @tremes @openshift-bot @openshift-merge-robot