Add log filtering functionality #71

detro · 2022-07-04T16:46:48Z

Closes #17

This is a general purpose solution, to enable provider developers to setup log filtering at runtime. Both log omission and log masking are supported.

Feature summary

For log omission (i.e. the log is never given to the underlying writer), it offers the following:

omit log by keys: if one or more given keys are met, the log is omitted from output
omit log matching regexp: if one of more of the given regular expressions are matched by the log message, the log is omitted from output
omit log matching string: simplified version of the regexp case

For log masking, it offers the following:

mask log value by keys: for keys that are matched within log arguments, the value is masked with ***
mask log matching regexp: every sub-string matching the regexp within the log message, are masked with ***
mask log matching string: simplified version of the regexp case

Feature coverage

The log filtering covers all the loggers in the package:

tflog provider root logger
tflog provider subsystems logger
tfsdklog provider logger
tfsdklog provider subsystems logger

bendbennett

LGTM. Couple of very minor comments.

bendbennett · 2022-07-05T07:43:59Z

internal/hclogutils/args.go


-		return result
+	for i := 0; i < len(args); i += 2 {


Although it looks like this should never happen, if len(args) is an odd/uneven number, then a key with no corresponding value could be appended to keys.

Perhaps for i := 0; i + 1 < len(args); i += 2 { could be used to avoid such a scenario?

I added tests for this as well

internal/hclogutils/args_test.go

internal/logging/filtering.go

bflad

Nice work so far -- a couple opening questions/comments before potentially diving deeper into things.

bflad · 2022-07-11T19:21:53Z

tflog/provider.go

+func omitOrMask(ctx context.Context, logger hclog.Logger, msg *string, additionalFields []map[string]interface{}) ([]interface{}, bool) {
+	tfLoggerOpts := logging.GetProviderRootTFLoggerOpts(ctx)
+	additionalArgs := hclogutils.MapsToArgs(additionalFields...)
+	impliedArgs := logger.ImpliedArgs()


Could we save a lot of slice trouble by keeping everything as maps with terraform-plugin-log? e.g. Keeping additionalFields as []map[string]interface{} (or better yet requiring the merged additional fields map) and writing the inverse of hclogutils.MapsToArgs() if we ever need to interrogate the underlying (Logger).ImpliedArgs()? That said, we might be able to avoid ImpliedArgs() completely, if we also saved the With() information similar to this filtering information.

Doing the filtering on the maps, would force the logic to convert the ImpliedArgs to map, before it can be filtered.
So I did the opposite, given that the underlying library requires to go in the args = [][]interface{} direction.

Right, but in my opinion it is more difficult to work with the slices correctly since it is an odd way to store paired data, when a map gives you direct access to paired keys and values. I think it would save a lot of code complexity and readability, such as not needing to loop through all slice elements to find a key to mask its value, to let the slice stuff be a detail we don't have to worry about except for converting to/from.

It would be nice to avoid ImpliedArgs() completely, but that change can certainly happen outside the scope of adding filters.

I agree that the ImpliedArgs() (that ultimately dictate this) are a sub-par thing from hclog. But I didn't want to expand my work to have to also alter With() so to always keep those key/value pairs stored in the context.

And writing the couple of i += 2 for loops was relatively easy.

We can refactor that later I guess.

There was no need to refactor With() now (we could've introduced the slice to map converter temporarily), but like you say, it can be refactored later.

I'll create a follow up ticket to do the refactoring and simplification: iterating over the keys of a map is definitely the most desirable way to go. Just focused on getting the functionality done and working on this one.

tflog/provider.go

internal/logging/provider.go

tflog/provider.go

tflog/provider_test.go

It takes `ImplicitArgs()` from `hclog.Logger`, and returns the keys of the k/v pairs slice

…ckage

…onfiguration in Context

- tflog root logger - tflog subsystem logger - tfsdklog root logger - tfsdklog subsystem logger

bflad

I don't agree with the readability and complexity of using slices throughout over maps, but the implementation works and that can be refactored later. Looks good to me, otherwise. 🚀

detro · 2022-07-13T18:00:56Z

Will add a CHANGELOG entry for 0.5.0 and merge it

detro · 2022-07-14T09:42:15Z

@bflad as agreed, here is an issue to track the work to refactor the whole use of implicit args #75

.changelog/71.txt

…elease

github-actions · 2022-08-14T02:38:27Z

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.

detro requested a review from a team as a code owner July 4, 2022 16:46

detro added the enhancement New feature or request label Jul 4, 2022

detro added this to the v0.5.0 milestone Jul 4, 2022

bendbennett approved these changes Jul 5, 2022

View reviewed changes

bflad reviewed Jul 11, 2022

View reviewed changes

detro force-pushed the detro/17-log_filtering branch from a77fef1 to 5a3bd75 Compare July 13, 2022 16:05

detro mentioned this pull request Jul 13, 2022

Prefer using raw fixtures in testing #74

Merged

detro force-pushed the detro/17-log_filtering branch from 5a3bd75 to 8e9c63e Compare July 13, 2022 16:16

Ivan De Marino added 5 commits July 13, 2022 17:27

New utility function in internal/hclogutils: ArgsToKeys

1196825

It takes `ImplicitArgs()` from `hclog.Logger`, and returns the keys of the k/v pairs slice

Implementing the business logic of filtering in internal/logging pa…

49e7709

…ckage

Defining utilities to create loggerKeys for storing log filtering c…

15e3dad

…onfiguration in Context

Exposing log filtering on all offerings of this library

48a8bdb

- tflog root logger - tflog subsystem logger - tfsdklog root logger - tfsdklog subsystem logger

Missed an import

a1e51ea

detro force-pushed the detro/17-log_filtering branch from a51f677 to a1e51ea Compare July 13, 2022 16:27

bflad approved these changes Jul 13, 2022

View reviewed changes

detro mentioned this pull request Jul 14, 2022

Rework the SetField(key, val) function to use a dedicated map[string]interface{} to store implicit arguments #75

Closed

bendbennett approved these changes Jul 14, 2022

View reviewed changes

.changelog/71.txt Outdated Show resolved Hide resolved

.changelog/71.txt Outdated Show resolved Hide resolved

.changelog/71.txt Outdated Show resolved Hide resolved

.changelog/71.txt Outdated Show resolved Hide resolved

Adding CHANGELOG entries for log filtering and preparing for v0.5.0 r…

d23e161

…elease

detro force-pushed the detro/17-log_filtering branch from 84ad741 to d23e161 Compare July 14, 2022 12:05

detro merged commit 6c641bd into main Jul 14, 2022

detro deleted the detro/17-log_filtering branch July 14, 2022 12:06

github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add log filtering functionality #71

Add log filtering functionality #71

detro commented Jul 4, 2022 •

edited

Loading

bendbennett left a comment

bendbennett Jul 5, 2022

detro Jul 5, 2022

bflad left a comment

bflad Jul 11, 2022

detro Jul 12, 2022

bflad Jul 12, 2022

detro Jul 13, 2022

bflad Jul 13, 2022 •

edited

Loading

detro Jul 13, 2022

bflad left a comment

detro commented Jul 13, 2022

detro commented Jul 14, 2022

github-actions bot commented Aug 14, 2022

Add log filtering functionality #71

Add log filtering functionality #71

Conversation

detro commented Jul 4, 2022 • edited Loading

Feature summary

Feature coverage

bendbennett left a comment

Choose a reason for hiding this comment

bendbennett Jul 5, 2022

Choose a reason for hiding this comment

detro Jul 5, 2022

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad Jul 11, 2022

Choose a reason for hiding this comment

detro Jul 12, 2022

Choose a reason for hiding this comment

bflad Jul 12, 2022

Choose a reason for hiding this comment

detro Jul 13, 2022

Choose a reason for hiding this comment

bflad Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

detro Jul 13, 2022

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

detro commented Jul 13, 2022

detro commented Jul 14, 2022

github-actions bot commented Aug 14, 2022

detro commented Jul 4, 2022 •

edited

Loading

bflad Jul 13, 2022 •

edited

Loading