Refactor into internal package. #10
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
An upcoming change is going to introduce a logging sink, which is going to be a root logger that both our provider and SDK logs need to be sub-loggers from, so they can inherit its filtering. This means that we can no longer have the loggers siloed off from each other, or at least need the sink to be accessible to both. But that got me thinking: does it make _sense_ to have the loggers in their own packages, or should the packages be boundaries around intended audience? The number of "don't use this unless you're building an SDK" comments in tflog make me sad, and make me wonder if it's better to just say "tflog is for provider devs, tfsdklog is for SDK devs". This PR is that. It refactors out the shared stuff between both loggers into an internal/logging package, then uses that in tflog and tfsdklog. It also moves things around so that only the functions we expect provider devs to use are in the tflog package, only the functions we expect SDK devs to use are in the tfsdklog package, and functions that are only meant to be used by terraform-plugin-log (for testing, etc.) are siloed off in the internal/testing package. This feels much cleaner and less repetitive, and feels like a better interface for provider devs and SDK devs alike.
paddycarver
added a commit
that referenced
this pull request
Sep 2, 2021
Starting to address #6. This is just the start of the work, but it gives us something to build on. Basically, what we're trying to do is build log sink functionality into terraform-plugin-log. Normally, Terraform acts as a log sink, gathering up logs from core and from all the plugins, filtering them appropriately, and combining them into a single log output stream, and deciding where to send that stream. However, when running acceptance tests, the plugin is actually the longest-running process, and so this model breaks down. Instead, we need to have the plugin act as the sink, gathering the logs from core and the plugin and other plugins, combining them into a single stream, filtering the stream, and deciding where to output the stream. Rather than implement this functionality multiple times in different SDKs, it makes more sense to add it to the tfsdklog package and just call it from the test frameworks. There are a few parts to this. First, we need a new sink logger that all other loggers are derived from. It should read environment variables to determine what level of output is desired, what format of output is desired, and where to send that output. Second, we should make all our loggers be sub-loggers of that sink when the sink is set. Third, we should make sure our sub-loggers can only log at levels equal to or higher than the level of this new sink logger when it's set. And finally, we should provide functionality to process hclog JSON output (like from terraform when TF_LOG=json) and route it through this sink logger, so we can combine the log output of the CLI and other providers with the log output native to the process. This commit falls short in a few places: * sub-loggers aren't limited to equal-or-higher levels from the sink logger yet. * none of this has been tested, even manually. It builds? * we need to check that all the environment variables we need to be honoring are getting honored for the provider under test, the CLI, and the providers the CLI launches. This depends on #10.
bflad
approved these changes
Sep 3, 2021
Replace all uses of ProviderSpace with Provider, and move WithStderrFromInit back so it doesn't show up in the diff for no reason.
paddycarver
added a commit
that referenced
this pull request
Sep 4, 2021
Starting to address #6. This is just the start of the work, but it gives us something to build on. Basically, what we're trying to do is build log sink functionality into terraform-plugin-log. Normally, Terraform acts as a log sink, gathering up logs from core and from all the plugins, filtering them appropriately, and combining them into a single log output stream, and deciding where to send that stream. However, when running acceptance tests, the plugin is actually the longest-running process, and so this model breaks down. Instead, we need to have the plugin act as the sink, gathering the logs from core and the plugin and other plugins, combining them into a single stream, filtering the stream, and deciding where to output the stream. Rather than implement this functionality multiple times in different SDKs, it makes more sense to add it to the tfsdklog package and just call it from the test frameworks. There are a few parts to this. First, we need a new sink logger that all other loggers are derived from. It should read environment variables to determine what level of output is desired, what format of output is desired, and where to send that output. Second, we should make all our loggers be sub-loggers of that sink when the sink is set. Third, we should make sure our sub-loggers can only log at levels equal to or higher than the level of this new sink logger when it's set. And finally, we should provide functionality to process hclog JSON output (like from terraform when TF_LOG=json) and route it through this sink logger, so we can combine the log output of the CLI and other providers with the log output native to the process. This commit falls short in a few places: * sub-loggers aren't limited to equal-or-higher levels from the sink logger yet. * none of this has been tested, even manually. It builds? * we need to check that all the environment variables we need to be honoring are getting honored for the provider under test, the CLI, and the providers the CLI launches. This depends on #10.
paddycarver
added a commit
that referenced
this pull request
Sep 4, 2021
paddycarver
added a commit
that referenced
this pull request
Sep 4, 2021
Starting to address #6. This is just the start of the work, but it gives us something to build on. Basically, what we're trying to do is build log sink functionality into terraform-plugin-log. Normally, Terraform acts as a log sink, gathering up logs from core and from all the plugins, filtering them appropriately, and combining them into a single log output stream, and deciding where to send that stream. However, when running acceptance tests, the plugin is actually the longest-running process, and so this model breaks down. Instead, we need to have the plugin act as the sink, gathering the logs from core and the plugin and other plugins, combining them into a single stream, filtering the stream, and deciding where to output the stream. Rather than implement this functionality multiple times in different SDKs, it makes more sense to add it to the tfsdklog package and just call it from the test frameworks. There are a few parts to this. First, we need a new sink logger that all other loggers are derived from. It should read environment variables to determine what level of output is desired, what format of output is desired, and where to send that output. Second, we should make all our loggers be sub-loggers of that sink when the sink is set. Third, we should make sure our sub-loggers can only log at levels equal to or higher than the level of this new sink logger when it's set. And finally, we should provide functionality to process hclog JSON output (like from terraform when TF_LOG=json) and route it through this sink logger, so we can combine the log output of the CLI and other providers with the log output native to the process. This commit falls short in a few places: * sub-loggers aren't limited to equal-or-higher levels from the sink logger yet. * none of this has been tested, even manually. It builds? * we need to check that all the environment variables we need to be honoring are getting honored for the provider under test, the CLI, and the providers the CLI launches. This depends on #10.
paddycarver
added a commit
that referenced
this pull request
Sep 4, 2021
Turns out it's hard to configure these things if you have no way of building a slice of the options.
paddycarver
added a commit
that referenced
this pull request
Sep 4, 2021
Starting to address #6. This is just the start of the work, but it gives us something to build on. Basically, what we're trying to do is build log sink functionality into terraform-plugin-log. Normally, Terraform acts as a log sink, gathering up logs from core and from all the plugins, filtering them appropriately, and combining them into a single log output stream, and deciding where to send that stream. However, when running acceptance tests, the plugin is actually the longest-running process, and so this model breaks down. Instead, we need to have the plugin act as the sink, gathering the logs from core and the plugin and other plugins, combining them into a single stream, filtering the stream, and deciding where to output the stream. Rather than implement this functionality multiple times in different SDKs, it makes more sense to add it to the tfsdklog package and just call it from the test frameworks. There are a few parts to this. First, we need a new sink logger that all other loggers are derived from. It should read environment variables to determine what level of output is desired, what format of output is desired, and where to send that output. Second, we should make all our loggers be sub-loggers of that sink when the sink is set. Third, we should make sure our sub-loggers can only log at levels equal to or higher than the level of this new sink logger when it's set. And finally, we should provide functionality to process hclog JSON output (like from terraform when TF_LOG=json) and route it through this sink logger, so we can combine the log output of the CLI and other providers with the log output native to the process. This commit falls short in a few places: * sub-loggers aren't limited to equal-or-higher levels from the sink logger yet. * none of this has been tested, even manually. It builds? * we need to check that all the environment variables we need to be honoring are getting honored for the provider under test, the CLI, and the providers the CLI launches. This depends on #10.
paddycarver
added a commit
that referenced
this pull request
Sep 4, 2021
paddycarver
added a commit
that referenced
this pull request
Sep 7, 2021
Starting to address #6. This is just the start of the work, but it gives us something to build on. Basically, what we're trying to do is build log sink functionality into terraform-plugin-log. Normally, Terraform acts as a log sink, gathering up logs from core and from all the plugins, filtering them appropriately, and combining them into a single log output stream, and deciding where to send that stream. However, when running acceptance tests, the plugin is actually the longest-running process, and so this model breaks down. Instead, we need to have the plugin act as the sink, gathering the logs from core and the plugin and other plugins, combining them into a single stream, filtering the stream, and deciding where to output the stream. Rather than implement this functionality multiple times in different SDKs, it makes more sense to add it to the tfsdklog package and just call it from the test frameworks. There are a few parts to this. First, we need a new sink logger that all other loggers are derived from. It should read environment variables to determine what level of output is desired, what format of output is desired, and where to send that output. Second, we should make all our loggers be sub-loggers of that sink when the sink is set. Third, we should make sure our sub-loggers can only log at levels equal to or higher than the level of this new sink logger when it's set. And finally, we should provide functionality to process hclog JSON output (like from terraform when TF_LOG=json) and route it through this sink logger, so we can combine the log output of the CLI and other providers with the log output native to the process. This commit falls short in a few places: * sub-loggers aren't limited to equal-or-higher levels from the sink logger yet. * none of this has been tested, even manually. It builds? * we need to check that all the environment variables we need to be honoring are getting honored for the provider under test, the CLI, and the providers the CLI launches. This depends on #10.
paddycarver
added a commit
that referenced
this pull request
Sep 7, 2021
paddycarver
added a commit
that referenced
this pull request
Sep 15, 2021
Starting to address #6. This is just the start of the work, but it gives us something to build on. Basically, what we're trying to do is build log sink functionality into terraform-plugin-log. Normally, Terraform acts as a log sink, gathering up logs from core and from all the plugins, filtering them appropriately, and combining them into a single log output stream, and deciding where to send that stream. However, when running acceptance tests, the plugin is actually the longest-running process, and so this model breaks down. Instead, we need to have the plugin act as the sink, gathering the logs from core and the plugin and other plugins, combining them into a single stream, filtering the stream, and deciding where to output the stream. Rather than implement this functionality multiple times in different SDKs, it makes more sense to add it to the tfsdklog package and just call it from the test frameworks. There are a few parts to this. First, we need a new sink logger that all other loggers are derived from. It should read environment variables to determine what level of output is desired, what format of output is desired, and where to send that output. Second, we should make all our loggers be sub-loggers of that sink when the sink is set. Third, we should make sure our sub-loggers can only log at levels equal to or higher than the level of this new sink logger when it's set. And finally, we should provide functionality to process hclog JSON output (like from terraform when TF_LOG=json) and route it through this sink logger, so we can combine the log output of the CLI and other providers with the log output native to the process. This commit falls short in a few places: * sub-loggers aren't limited to equal-or-higher levels from the sink logger yet. * none of this has been tested, even manually. It builds? * we need to check that all the environment variables we need to be honoring are getting honored for the provider under test, the CLI, and the providers the CLI launches. This depends on #10.
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An upcoming change is going to introduce a logging sink, which is going
to be a root logger that both our provider and SDK logs need to be
sub-loggers from, so they can inherit its filtering. This means that we
can no longer have the loggers siloed off from each other, or at least
need the sink to be accessible to both.
But that got me thinking: does it make sense to have the loggers in
their own packages, or should the packages be boundaries around intended
audience? The number of "don't use this unless you're building an SDK"
comments in tflog make me sad, and make me wonder if it's better to just
say "tflog is for provider devs, tfsdklog is for SDK devs".
This PR is that. It refactors out the shared stuff between both loggers
into an internal/logging package, then uses that in tflog and tfsdklog.
It also moves things around so that only the functions we expect
provider devs to use are in the tflog package, only the functions we
expect SDK devs to use are in the tfsdklog package, and functions that
are only meant to be used by terraform-plugin-log (for testing, etc.)
are siloed off in the internal/testing package.
This feels much cleaner and less repetitive, and feels like a better
interface for provider devs and SDK devs alike.