From bcb8cf1638ecd7cb1ff91249946b887df0b97782 Mon Sep 17 00:00:00 2001 From: Todd Baert Date: Wed, 4 Sep 2024 07:40:06 -0400 Subject: [PATCH 1/4] feat: logging requirements and logging hook (#269) As discussed in recent meetings, we've had complaints about this from multiple sources. This PR adds a stipulation that no logging is done by the SDK during flag evaluation. It also defines a very simple `logging hook` concept as an included utility. These should be very simple to write and will provide all the needed logging but give authors more control than built in log statements. It will also be a very nice intro to the hooks concept for users. Here's the Java implementation: https://github.com/open-feature/java-sdk/pull/1084 --------- Signed-off-by: Todd Baert --- specification.json | 4 ++-- .../appendix-a-included-utilities.md | 23 ++++++++++++++++++- specification/sections/01-flag-evaluation.md | 10 ++++++-- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/specification.json b/specification.json index 1a3b3d31..652c2c98 100644 --- a/specification.json +++ b/specification.json @@ -233,8 +233,8 @@ { "id": "Requirement 1.4.11", "machine_id": "requirement_1_4_11", - "content": "In the case of abnormal execution, the client SHOULD log an informative error message.", - "RFC 2119 keyword": "SHOULD", + "content": "Methods, functions, or operations on the client SHOULD NOT write log messages.", + "RFC 2119 keyword": "SHOULD NOT", "children": [] }, { diff --git a/specification/appendix-a-included-utilities.md b/specification/appendix-a-included-utilities.md index e972d01c..269fe168 100644 --- a/specification/appendix-a-included-utilities.md +++ b/specification/appendix-a-included-utilities.md @@ -11,7 +11,7 @@ This document contains requirements for auxiliary utilities provided by the SDK, ## In-memory provider -> Language-specific OpenFeature SDK implementations **SHOULD** expose an in-memory provider built into the SDK. +> OpenFeature SDK implementations **SHOULD** provide an `in-memory provider`. The in-memory provider is intended to be used for testing; SDK consumers may use it for their use cases. Hence, the packaging, naming, and access modifiers must be set appropriately. @@ -360,3 +360,24 @@ Providers can contain metadata. The Multi-Provider will make that metadata avail }, } ``` + +## Logging Hook + +> OpenFeature SDK implementations **SHOULD** provide a `logging hook`. + +The logging hook is a hook which logs messages during the flag evaluation life-cycle as described below: + +| Stage | Logged data | +| ------- | --------------------------------------------------------------------------------------------------------------------------------------------- | +| before | `stage`, `domain`, `provider_name`, `flag_key`, `default_value` and `evaluation_context` (serialized, opt-in) | +| after | `stage`, `domain`, `provider_name`, `flag_key`, `default_value`, `evaluation_context` (serialized, opt-in), `reason`, `variant` and `value` | +| error | `stage`, `domain`, `provider_name`, `flag_key`, `default_value`, `evaluation_context` (serialized, opt-in), `error code`, and `error_message` | +| finally | N/A | + +> The evaluation context **SHOULD** only be logged if an associated option indicates so. + +The can be a constructor option or similar, for example: `boolean printContext`. + +> If logging the evaluation context is enabled, it **MUST** be printed in such a way that it's human readable. + +> If the logger abstraction in the SDK supports a log level concept, the appropriate log level **SHOULD** be used for each stage (before/after: info, error: error). diff --git a/specification/sections/01-flag-evaluation.md b/specification/sections/01-flag-evaluation.md index 43b72000..5d620d99 100644 --- a/specification/sections/01-flag-evaluation.md +++ b/specification/sections/01-flag-evaluation.md @@ -353,9 +353,15 @@ Configuration code includes code to set the provider, instantiate providers, and #### Requirement 1.4.11 -> In the case of abnormal execution, the client **SHOULD** log an informative error message. +> Methods, functions, or operations on the client **SHOULD NOT** write log messages. -Implementations may define a standard logging interface that can be supplied as an optional argument to the client creation function, which may wrap standard logging functionality of the implementation language. +The client methods (particularly the evaluation methods) run in hot code paths. +Logging (even at error level) can cause a huge volume of log entries. +For example, in a circumstance in which an application expecting a particular flag to exist is deployed in advance of that flag's being defined in the management system, logs can become inundated with `FLAG_NOT_FOUND` messages and related stack traces. +Logging in these code paths is highly discouraged. +Application authors can attach a [logging hook](../appendix-a-included-utilities.md#logging-hook) or author their own custom logging hook(s) to help with debugging or satisfy their particular logging needs. + +Logging is encouraged in functions to do with configuration, initialization, shutdown, etc. #### Requirement 1.4.12 From 50b09898ca78ba6c20143cbcf6407daef01c72c0 Mon Sep 17 00:00:00 2001 From: Todd Baert Date: Wed, 4 Sep 2024 10:34:29 -0400 Subject: [PATCH 2/4] fixup: typo and improve supporting text in logging hook Signed-off-by: Todd Baert --- specification/appendix-a-included-utilities.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/specification/appendix-a-included-utilities.md b/specification/appendix-a-included-utilities.md index 269fe168..d382ba5a 100644 --- a/specification/appendix-a-included-utilities.md +++ b/specification/appendix-a-included-utilities.md @@ -376,8 +376,12 @@ The logging hook is a hook which logs messages during the flag evaluation life-c > The evaluation context **SHOULD** only be logged if an associated option indicates so. -The can be a constructor option or similar, for example: `boolean printContext`. +This can be a constructor option or similar, for example: `boolean printContext`. > If logging the evaluation context is enabled, it **MUST** be printed in such a way that it's human readable. -> If the logger abstraction in the SDK supports a log level concept, the appropriate log level **SHOULD** be used for each stage (before/after: info, error: error). +Consider printing the evaluation context as a stringified JSON object, or using some other format that allows the nested properties to be easily read. + +> If the logger abstraction in the SDK supports a log level concept, the appropriate log level **SHOULD** be used for each stage (before/after: debug/info, error: error). + +Consider using `debug` or `info` levels for the `before` and `after` stages, and the `error` level for the `error` stage. From 7f7830b5d832f73686d3937c10a20ab39335b196 Mon Sep 17 00:00:00 2001 From: Todd Baert Date: Wed, 4 Sep 2024 10:35:11 -0400 Subject: [PATCH 3/4] fix: underscore instead of space Signed-off-by: Todd Baert --- specification/appendix-a-included-utilities.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specification/appendix-a-included-utilities.md b/specification/appendix-a-included-utilities.md index d382ba5a..f42d54c0 100644 --- a/specification/appendix-a-included-utilities.md +++ b/specification/appendix-a-included-utilities.md @@ -371,7 +371,7 @@ The logging hook is a hook which logs messages during the flag evaluation life-c | ------- | --------------------------------------------------------------------------------------------------------------------------------------------- | | before | `stage`, `domain`, `provider_name`, `flag_key`, `default_value` and `evaluation_context` (serialized, opt-in) | | after | `stage`, `domain`, `provider_name`, `flag_key`, `default_value`, `evaluation_context` (serialized, opt-in), `reason`, `variant` and `value` | -| error | `stage`, `domain`, `provider_name`, `flag_key`, `default_value`, `evaluation_context` (serialized, opt-in), `error code`, and `error_message` | +| error | `stage`, `domain`, `provider_name`, `flag_key`, `default_value`, `evaluation_context` (serialized, opt-in), `error_code`, and `error_message` | | finally | N/A | > The evaluation context **SHOULD** only be logged if an associated option indicates so. From 80ceaa0465ac17ad88856550f284a93e36a7f382 Mon Sep 17 00:00:00 2001 From: Todd Baert Date: Wed, 4 Sep 2024 12:06:55 -0400 Subject: [PATCH 4/4] fix: lint Signed-off-by: Todd Baert --- specification/appendix-a-included-utilities.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/specification/appendix-a-included-utilities.md b/specification/appendix-a-included-utilities.md index f42d54c0..ce92dd67 100644 --- a/specification/appendix-a-included-utilities.md +++ b/specification/appendix-a-included-utilities.md @@ -384,4 +384,4 @@ Consider printing the evaluation context as a stringified JSON object, or using > If the logger abstraction in the SDK supports a log level concept, the appropriate log level **SHOULD** be used for each stage (before/after: debug/info, error: error). -Consider using `debug` or `info` levels for the `before` and `after` stages, and the `error` level for the `error` stage. +Consider using `debug` or `info` levels for the `before` and `after` stages, and the `error` level for the `error` stage.