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

tfsdklog: Introduce outside context level checks for logging functions #149

Merged
merged 1 commit into from
May 31, 2023

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Apr 19, 2023

Reference: hashicorp/terraform-plugin-framework#721

This change introduces log level checks outside the context.Context of a request to improve the performance of logging functions when logs would not be emitted at the configured level. Almost exclusively, logging levels are not expected to change during provider (and therefore SDK) runtime since they are environment variable driven. Even if they did, this level check will either immediately constrict logging to match an updated higher level or cause additional context.Context reads with an updated lower level, which is no different than the current behavior.

The following benchmark was ran prior to the introduction of the level checks and mutexes:

BenchmarkSubsystemTraceDisabled-10            4616656               258.0 ns/op
BenchmarkSubsystemTraceEnabled-10              936865              1138 ns/op

After the addition of level checks and mutexes:

BenchmarkSubsystemTraceDisabled-10           86043471                13.78 ns/op
BenchmarkSubsystemTraceEnabled-10              906649              1137 ns/op

This enhancement could also be considered for provider loggers, however SDK logging tends to be more prevalent in practice, so this only targets the tfsdklog package handling.

@bflad bflad added the enhancement New feature or request label Apr 19, 2023
@bflad bflad added this to the v0.9.0 milestone Apr 19, 2023
@bflad bflad requested a review from a team as a code owner April 19, 2023 00:38
Reference: hashicorp/terraform-plugin-framework#721

This change introduces log level checks outside the `context.Context` of a request to improve the performance of logging functions when logs would not be emitted at the configured level. Almost exclusively, logging levels are not expected to change during provider (and therefore SDK) runtime since they are environment variable driven. Even if they did, this level check will either immediately constrict logging to match an updated higher level or cause additional `context.Context` reads with an updated lower level, which is no different than the current behavior.

The following benchmark was ran prior to the introduction of the level checks and mutexes:

```
BenchmarkSubsystemTraceDisabled-10            4616656               258.0 ns/op
BenchmarkSubsystemTraceEnabled-10              936865              1138 ns/op
```

After the addition of level checks and mutexes:

```
BenchmarkSubsystemTraceDisabled-10           86043471                13.78 ns/op
BenchmarkSubsystemTraceEnabled-10              906649              1137 ns/op
```

This enhancement could also be considered for provider loggers, however SDK logging tends to be more prevalent in practice, so this only targets the `tfsdklog` package handling.
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

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

syntactically looks good 👍🏻 , will be interesting to hear how the performance improves from the original issue

@bflad
Copy link
Contributor Author

bflad commented May 31, 2023

We haven't heard back from the original reporter for awhile now, but we'd also like to cut a framework release. I'm going to optimistically merge this in so we can cut a release of this module and update the dependency in terraform-plugin-framework, then we can reassess things afterwards.

@bflad bflad merged commit baab61c into main May 31, 2023
@bflad bflad deleted the bflad/tfsdklog-log-level-checks branch May 31, 2023 21:19
@github-actions
Copy link

github-actions bot commented Jul 1, 2023

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants