Shortcut logging when the level isn't active #722
Conversation
Introduce a resource that can manage entries in bulk, which: 1. Solves terraform performance issues when dealing with a lot of entries 2. Reduces pressure on our API when working with entries To achieve this we've forked the terraform sdk framework to improve performance when logging, which is necessary to avoid massive plan times on large nested objects. Relevant links are: - PR for logging change (hashicorp/terraform-plugin-framework#722) - Issue in upstream (hashicorp/terraform-plugin-framework#721)
Introduce a resource that can manage entries in bulk, which: 1. Solves terraform performance issues when dealing with a lot of entries 2. Reduces pressure on our API when working with entries To achieve this we've forked the terraform sdk framework to improve performance when logging, which is necessary to avoid massive plan times on large nested objects. Relevant links are: - PR for logging change (hashicorp/terraform-plugin-framework#722) - Issue in upstream (hashicorp/terraform-plugin-framework#721)
Introduce a resource that can manage entries in bulk, which: 1. Solves terraform performance issues when dealing with a lot of entries 2. Reduces pressure on our API when working with entries To achieve this we've forked the terraform sdk framework to improve performance when logging, which is necessary to avoid massive plan times on large nested objects. Relevant links are: - PR for logging change (hashicorp/terraform-plugin-framework#722) - Issue in upstream (hashicorp/terraform-plugin-framework#721)
Introduce a resource that can manage entries in bulk, which: 1. Solves terraform performance issues when dealing with a lot of entries 2. Reduces pressure on our API when working with entries To achieve this we've forked the terraform sdk framework to improve performance when logging, which is necessary to avoid massive plan times on large nested objects. Relevant links are: - PR for logging change (hashicorp/terraform-plugin-framework#722) - Issue in upstream (hashicorp/terraform-plugin-framework#721)
This commit addresses this issue: hashicorp#721 When working with large terraform resource attributes, logging can become the primary cost to running plan/apply. This is because the context logger used by tfsdklog is very expensive, the SDK framework itself is very verbose, and even the log level means Debug/Trace logs won't be emitted we still build the log. As we log via the internal logging package anyway, we can avoid this performance issue by maintaining a package-local log-level which is initialised at the same time as we init the contextual logger. Then our logging package can avoid calling into the tfsdklog (which is where we pay the performance penalty) unless it believes the level is active, which in most cases avoids 99% of the log cost.
lawrencejones
force-pushed
the
lawrence-shortcut-logging
branch
from
b36f4a1
to
a39a43f
Compare
There was a problem hiding this comment.
Hi @lawrencejones 👋 Thank you for submitting this.
We can certainly introduce some additional logic here to check the logging level before doing all the work to fetch the logger from context.Context, however the framework's logging environment variable level is not the only one that influences which level the logger is running at. For example, the "global" TF_LOG environment variable also will enable logging.
I think what we'll want here is:
- Ensuring the existing logging testing is passing
- terraform-plugin-log loggers to expose their effective logging level based on all its prior configuration
- For that logging level to be a terraform-plugin-log type, rather than falling back to the hclog type
- Releasing that and switching the implementation here
No worries if this is more work than expected here. If you are interested though, it might be best to raise an upstream issue in terraform-plugin-log to expose the effective logging level.
|
Please refer to #721 (comment). |
|
Thank you for this submission. Superseded by upstream terraform-plugin-log changes via #744 so we can roll this out across the other SDKs as well -- if we still find there are performance issues, we can determine next steps in a fresh new issue. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This commit addresses this issue:
#721
When working with large terraform resource attributes, logging can become the primary cost to running plan/apply. This is because the context logger used by tfsdklog is very expensive, the SDK framework itself is very verbose, and even the log level means Debug/Trace logs won't be emitted we still build the log.
As we log via the internal logging package anyway, we can avoid this performance issue by maintaining a package-local log-level which is initialised at the same time as we init the contextual logger. Then our logging package can avoid calling into the tfsdklog (which is where we pay the performance penalty) unless it believes the level is active, which in most cases avoids 99% of the log cost.