Add SPLUNK_REALM to instrumentation library configuration
#148
Conversation
| | `SPLUNK_TRACE_RESPONSE_HEADER_ENABLED` (`true`) | Whether `Server-Timing` header is added to HTTP responses. [2] | | ||
| | `SPLUNK_METRICS_ENDPOINT` () | Endpoint for metrics data ingest. | | ||
| | `SPLUNK_METRICS_ENDPOINT` () | Endpoint for metrics data ingest. | | ||
| | `SPLUNK_REALM` (`none`) | Which realm to send exported data. [2] | |
There was a problem hiding this comment.
Other vars don't have a none default value and instead just default to nothing/empty string. Do we need a special default value ("none" string literal) for SPLUNK_REALM?
There was a problem hiding this comment.
I think we want to avoid using empty strings in general and use none in such cases in general: open-telemetry/opentelemetry-specification#2102
It's also consistent with setting exporters, propagators (was that PR merged already?)
There was a problem hiding this comment.
That makes sense as it allows users to easily "unset" a variable that has a non-none default but in case the variable does not have a default, do we want to specify none as the default value or just say it has no default? I know it does not make a difference in implementation but when reading the spec it can be confusing when some values do not have a default value while others have none as default value while both having the same net effect.
There was a problem hiding this comment.
In other words, none does not have any special meaning and is an alternate way to setting a variable to an empty string or removing the variable from the env. Keeping that in mind, we should either have none as default value for all vars that don't have a default or not have it as default anywhere for the sake of consistency.
Either way it does not affect implementations and the net effect is the same so not a hard blocker for me.
There was a problem hiding this comment.
Other vars don't have a
nonedefault value and instead just default to nothing/empty string
@owais Can you give an example of such enum-type configuration, please?
There was a problem hiding this comment.
None of these have any default values. How do we decide when something does not have a default vs when something has default set to none?
There was a problem hiding this comment.
none is for enum value (more).
Regarding provided examples, I think that only the Kubernetes configs could be enums. But they look more like dummy strings at the moment. Moreover, they are in experimental status.
There was a problem hiding this comment.
Yes, that makes sense and like I said this is not a hard blocker for me. Just that it can be confusing to me as an end user when I see a dozen options with some listing the default as none while others listing no default value and all of them behaving the same. As a consumer, I'd prefer a library to have uniform defaults/noop representations.
Feel free to resolve this if you'd like :)
| | `SPLUNK_TRACE_RESPONSE_HEADER_ENABLED` (`true`) | Whether `Server-Timing` header is added to HTTP responses. [2] | | ||
| | `SPLUNK_METRICS_ENDPOINT` () | Endpoint for metrics data ingest. | | ||
| | `SPLUNK_METRICS_ENDPOINT` () | Endpoint for metrics data ingest. | | ||
| | `SPLUNK_REALM` (`none`) | Which realm to send exported data. [2] | |
There was a problem hiding this comment.
In other words, none does not have any special meaning and is an alternate way to setting a variable to an empty string or removing the variable from the env. Keeping that in mind, we should either have none as default value for all vars that don't have a default or not have it as default anywhere for the sake of consistency.
Either way it does not affect implementations and the net effect is the same so not a hard blocker for me.
|
@MrAlias PTAL |
* Add behaviors Refactor profiling into: - Behaviors - Configuration - Semantic Conventions Behavior is new to cover specifics of some subset. Its goal is to drive consistent across GDI repositories. * Update specification/behaviors.md Co-authored-by: jason plumb <[email protected]> * Minor fixes in RUM spec (#155) - Add different statuses per semantic conventions section - everything but RUM is stable, RUM is feature-freeze (same as in the configuration doc) - Change os.type to os.name - I think it was a bug previously since both OTel spec and Android RUM actually use os.name * Add `SPLUNK_REALM` to instrumentation library configuration (#148) Addresses #140 * Add behaviors Refactor profiling into: - Behaviors - Configuration - Semantic Conventions Behavior is new to cover specifics of some subset. Its goal is to drive consistent across GDI repositories. * Resolve conflict - Resolve conflict the conflict - Remove the line number from the hyperlink as it may change. * Update specification/behaviors.md Co-authored-by: Robert Pająk <[email protected]> * Update specification/semantic_conventions.md Co-authored-by: Robert Pająk <[email protected]> * Update specification/semantic_conventions.md Co-authored-by: Robert Pająk <[email protected]> * Update specification/semantic_conventions.md Co-authored-by: Robert Pająk <[email protected]> * Update specification/semantic_conventions.md Co-authored-by: Robert Pająk <[email protected]> * Update specification/behaviors.md Co-authored-by: Robert Pająk <[email protected]> * Address feedback * Update CHANGELOG * Update CHANGELOG.md * Update specification/behaviors.md * Update behaviors.md Co-authored-by: jason plumb <[email protected]> Co-authored-by: Mateusz Rzeszutek <[email protected]> Co-authored-by: Robert Pająk <[email protected]>
Addresses #140