Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Use host max log level when initializing the RuntimeLogger#8655

Merged
2 commits merged intomasterfrom
bkchr-host-max-log-level
Apr 23, 2021
Merged

Use host max log level when initializing the RuntimeLogger#8655
2 commits merged intomasterfrom
bkchr-host-max-log-level

Conversation

@bkchr
Copy link
Member

@bkchr bkchr commented Apr 22, 2021

This should fix performance problems introduced by logging under certain
circumstances. Before we always called into the host and the host was
doing the log filtering, now as the correct max log level is set, we
don't call into the host for every log line to check if it should be
logged. However, we would still call into the host to determine if
something should be logged when something=trace is given as we don't
forward the log targets that are enabled.

Fixes: #8611

This should fix performance problems introduced by logging under certain
circumstances. Before we always called into the host and the host was
doing the log filtering, now as the correct max log level is set, we
don't call into the host for every log line to check if it should be
logged. However, we would still call into the host to determine if
something should be logged when `something=trace` is given as we don't
forward the log targets that are enabled.
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Apr 22, 2021
@bkchr
Copy link
Member Author

bkchr commented Apr 22, 2021

@athei could you test if this fixes your benchmarking problem?

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

This fixes my benchmarking problem.

@bkchr bkchr added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. E4-newhostfunctions D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Apr 22, 2021
@bkchr bkchr marked this pull request as ready for review April 22, 2021 21:43
@shawntabrizi shawntabrizi requested a review from athei April 23, 2021 01:46
Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

LGTM

@shawntabrizi
Copy link
Member

bot merge

@ghost
Copy link

ghost commented Apr 23, 2021

Trying merge.

@ghost ghost merged commit bb22414 into master Apr 23, 2021
@ghost ghost deleted the bkchr-host-max-log-level branch April 23, 2021 13:22
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate increased contract instrumentation costs

4 participants