Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 47 additions & 8 deletions specification/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,8 @@
a deprecation warning and suggest an alternate method. For example:
```txt
jaeger-thrift-splunk trace exporter is deprecated and may be removed in a future major release. Use the default
OTLP exporter instead, or set the SPLUNK_REALM and SPLUNK_ACCESS_TOKEN environment variables to send
jaeger-thrift-splunk trace exporter is deprecated and may be removed in a future major release. Use the default
OTLP exporter instead, or set the SPLUNK_REALM and SPLUNK_ACCESS_TOKEN environment variables to send
telemetry directly to Splunk Observability Cloud.
```
Expand All @@ -250,12 +250,51 @@
In addition to environment variables, other ways of defining configuration also exist:
- [Java System
Properties](https://docs.oracle.com/javase/tutorial/essential/environment/sysprop.html):
These properties MUST match the environment variables converting to lower
case and replacing underscores with hyphens or periods. For example:
system property `splunk.trace-response-header.enabled` is equivalent to environment
variable `SPLUNK_TRACE_RESPONSE_HEADER_ENABLED`.
#### File based configuration

Check failure on line 253 in specification/configuration.md

View workflow job for this annotation

GitHub Actions / validate-documentation

Heading levels should only increment by one level at a time

specification/configuration.md:253 MD001/heading-increment Heading levels should only increment by one level at a time [Expected: h3; Actual: h4] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md001.md
OpenTelemetry's [declarative configuration](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/README.md#declarative-configuration) SHOULD be supported via [`OTEL_EXPERIMENTAL_CONFIG_FILE`](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk.md#via-otel_experimental_config_file) environment variable.

Check failure on line 255 in specification/configuration.md

View workflow job for this annotation

GitHub Actions / validate-documentation

Line length

specification/configuration.md:255:81 MD013/line-length Line length [Expected: 80; Actual: 402] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md013.md
In addition, Splunk specific configuration MUST have their own root-level configuration block named `splunk`.

Check failure on line 257 in specification/configuration.md

View workflow job for this annotation

GitHub Actions / validate-documentation

Line length

specification/configuration.md:257:81 MD013/line-length Line length [Expected: 80; Actual: 109] https://github.com/DavidAnson/markdownlint/blob/v0.38.0/doc/md013.md
The following is an example covering the common configuration values:
```yaml
splunk:
# Common values across language distributions.
realm: "eu0"
accessToken: "${SPLUNK_ACCESS_TOKEN}"
tracing:
enabled: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use disabled instead of enabled as this is the pattern used in delcarative config?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @Kielek comment is more correct regarding how we should handle enabling/disabling:
#355 (comment)

disabled in the declarative config is used to disable the SDK, but we don’t have for example a way to disable a trace/metric/logger providers, propagators or resource detectors.
So I’m pretty sure it should be if the node is present, it means it’s enabled. To disable it, you should remove the entire node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed it to be disabled, the configuration is already pretty hostile to users (with the exporter header, endpoint duplication), I'm not really keen on removing a simple field to toggle the whole signal.

If we're going the route of removing disabled / enabled field, then:

  • tracing / metrics / logs would be toggled via the existence of tracer_provider / meter_provider / logger_provider top level nodes
  • profiling would be toggled via the existence of distribution.splunk.profiling node
  • callgraphs would be toggled via the existence of distribution.splunk.callgraphs node

However as a compromise the disabled field under signals could be optional, where an unspecified value means the signal is on, so you'd explicitly have to write disabled: true in order to disable a signal without removing the whole config block (i.e. when you're debugging something).

Copy link
Contributor

Choose a reason for hiding this comment

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

without removing the whole config block (i.e. when you're debugging something).

One could simply comment out the whole code. I do not feel that we need this configuration option. I prefer to minimize the scope if possible; similarly to what we agreed with SPLUNK_REALM and SPLUNK_ACCESS_TOKEN.

responseHeaderEnabled: true # SPLUNK_TRACE_RESPONSE_HEADER_ENABLED
profiling:
enabled: true # SPLUNK_PROFILER_ENABLED
endpoint: "" # SPLUNK_PROFILER_LOGS_ENDPOINT
memoryProfilingEnabled: true # SPLUNK_PROFILER_MEMORY_ENABLED
callgraphs:
enabled: true # SPLUNK_SNAPSHOT_PROFILER_ENABLED
samplingInterval: 10 # SPLUNK_SNAPSHOT_SAMPLING_INTERVAL
selectionProbability: 0.01 # SPLUNK_SNAPSHOT_SAMPLING_INTERVAL
# Language specific configurations
runtime:
nodejs:
metrics:
instrumentationMetricsEnabled: true # SPLUNK_INSTRUMENTATION_METRICS_ENABLED

Choose a reason for hiding this comment

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

Why this kind of properties should not go under .instrumentation.nodejs ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that things related to instrumentations can be placed under the .instrumentation.nodejs node,
but the profiler call graphs and SPLUNK_TRACE_RESPONSE_HEADER_ENABLED should be made into a separate node .splunk or .vendors.splunk

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that SPLUNK_TRACE_RESPONSE_HEADER_ENABLED is also a kind of instrumentation 😉 At least in Go, we even have a dedicated instrumentation library which enables it 😉

runtimeMetrics:
enabled: true # SPLUNK_RUNTIME_METRICS_ENABLED
collectionInterval: 30000 # SPLUNK_RUNTIME_METRICS_COLLECTION_INTERVAL
autoInstrumentPackages:
- "MyApiService"
- "MyOtherService"
java:
...
```

#### [Java SystemProperties](https://docs.oracle.com/javase/tutorial/essential/environment/sysprop.html)

These properties MUST match the environment variables converting to lower
case and replacing underscores with hyphens or periods. For example:
system property `splunk.trace-response-header.enabled` is equivalent to environment
variable `SPLUNK_TRACE_RESPONSE_HEADER_ENABLED`.

### Snapshot Profiler

Expand Down
Loading