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

log.getChildLogEntries() does not return child log entries #4457

Closed
Walther opened this issue May 26, 2023 · 3 comments
Closed

log.getChildLogEntries() does not return child log entries #4457

Walther opened this issue May 26, 2023 · 3 comments

Comments

@Walther
Copy link
Contributor

Walther commented May 26, 2023

Bug report for garden internals, not directly user-facing but could affect some log-related behaviors externally too.

On the log class, the getChildLogEntries() method only returns this.entries, which contains the entries of the current log instance only:

getChildLogEntries() {
return this.entries
}

In order to actually get entries from possible child logs, you need to use getAllLogEntries() which fetches the entries from the root log.

Found when working on #4381, as the provider initialization logs did not appear in getLogMessages(garden.log), because the provider initialization code creates its own sub-log, and the sub-log is not correctly fetched within the getChildLogEntries() method called from getLogMessages()

@eysi09
Copy link
Collaborator

eysi09 commented May 30, 2023

This is actually the intended behaviour.

getAllLogEntries, as the name suggests, gets all log entries for that logger.

getChildLogEntries, gets the entries that belong to that specific log context. I.e., its "child" entries. The nomenclature comes from the time when the logger was a proper graph structure you could traverse.

I thought the name was clear enough so I intentionally didn't change it, but we can of course do so if this causes confusion. I think we do need to fix that getLogMessages function though and have it use getAllLogEntries().

@Walther
Copy link
Contributor Author

Walther commented May 30, 2023

Gotcha, thanks!
Would you mind if I do the following:

  • make all these relevant edits in the PR test(terraform): run tests for multiple tf versions #4381 where these were needed - even if technically this kind of changes could/should be done as a separate PR
  • remove the getRootLogMessages function I added in that PR to work around the issue
  • edit getLogMessages to use getAllLogEntries instead of getChildLogEntries, even if this means various test cases might get more logs back than before
  • rename getChildLogEntries to getLogEntries, as it only gets entries from the current log context you are calling the method on (as there is no graph anymore, and therefore no child logs to fetch messages from)

Any objections or suggestions for alternatives?

@Walther Walther removed the bug label May 30, 2023
@Walther Walther changed the title Bug: log.getChildLogEntries() does not return child log entries log.getChildLogEntries() does not return child log entries May 30, 2023
Walther added a commit that referenced this issue May 31, 2023
Walther added a commit that referenced this issue Jun 1, 2023
* test(terraform): run tests for multiple tf versions

* improvement: remove terraform lock files from git tree

* ci(circleci): re-enable plugins tests

* chore(terraform): fix hash for terraform 0.13.3 darwin amd64

* chore: terraform-plugin: 0.13 config updates for test projects

* chore: terraform-plugin: test updates for 0.13 config

* chore: terraform-plugin: fix provider tests

* chore: terraform-plugin: fix some action tests

* chore: terraform-plugin: fix 'sets the workspace before destroying' test

Co-authored-by: Steffen Neubauer <[email protected]>

* chore: terraform-plugin: fix rest of the skipped tests

Co-authored-by: Steffen Neubauer <[email protected]>

* chore: conftest plugin: temporarily disable tests, fix in another pr

* chore: conftest plugin: temporarily disable tests, fix in another pr

* chore: jib plugin: temporarily disable tests, fix in another pr

* chore: conftest test: fix import

* chore: jib plugin: temporarily disable tests, fix in another pr

* chore: terraform-plugin common tests: init before workspace commands

* chore: terraform-plugin: lockfile for both test projects

* chore: update unzipper

* refactor: improve log message methods for testing purposes

based on the feedback provided at #4457 (comment)

* chore: return the separate getRootLogMessages and getLogMessages

other unrelated tests were depending on the specific behavior.
we can always come back to refactor further.

* chore: disable terraform-plugin tests

The tests have been fixed and they pass locally. These tests fail in CI
because of #4467
Temporarily skipping these tests in order to unblock other PRs for the
other plugins and fixing their test setups, while keeping our CI green
for the test-plugins step.

* chore: terraform-plugin: remove lockfile

* chore: terraform-plugin tests: restore test-project-module

---------

Co-authored-by: Walther <[email protected]>
@Walther
Copy link
Contributor Author

Walther commented Jun 1, 2023

The following changes were made in this PR #4381

  • clarify the docstring on getLogMessages() and mention it only gets messages from the given context
  • add a separate getRootLogMessages() function which gets all log messages via the root log
  • rename getChildLogEntries to getLogEntries and add a docstring describing its behavior, in similar wording as the other functions around it

@Walther Walther closed this as completed Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants