-
Notifications
You must be signed in to change notification settings - Fork 124
[FileBasedConfiguration] Instrumentation Configuration #4492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FileBasedConfiguration] Instrumentation Configuration #4492
Conversation
...emetry.AutoInstrumentation/Configurations/FileBasedConfiguration/SetDocumentConfiguration.cs
Outdated
Show resolved
Hide resolved
...strumentation.Tests/Configurations/FileBased/Files/TestInstrumentationConfigurationFile.yaml
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.AutoInstrumentation/Configurations/FileBasedConfiguration/DotNetTraces.cs
Show resolved
Hide resolved
src/OpenTelemetry.AutoInstrumentation/Configurations/FileBasedConfiguration/DotNetMetrics.cs
Show resolved
Hide resolved
|
|
||
| foreach (var prop in properties) | ||
| { | ||
| var value = prop.GetValue(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that if a user wants to use file-based configuration, they will have to update their file to list every single instrumentation they want to enable? Can we instead add a bool enabled property to each entry, that way individual instrumentations can easily be disabled by setting enabled: false but if an instrumentation is not listed then it's on-by-default? That feels much more user-friendly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, it is implemented the same way as propagators and resource detectors, and this approach is compliant with the current specification. I believe we should keep the current approach until the specification changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, agreed that we should align with the specification. Do you have a link to the section describing the structure of those that I could look at?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is spec for detectors https://github.com/open-telemetry/opentelemetry-configuration/blob/main/examples/kitchen-sink.yaml#L844
Here is spec for propagators https://github.com/open-telemetry/opentelemetry-configuration/blob/main/examples/kitchen-sink.yaml#L541
Here is spec for instrumentations https://github.com/open-telemetry/opentelemetry-configuration/blob/main/examples/kitchen-sink.yaml#L902
Created issue for disabled/enabled in spec open-telemetry/opentelemetry-configuration#334
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behavior, even though it's in agreement with the spec does not seem like the desired behavior to me. In practice, I find that configuration files rarely get updated, and as new features are added they rarely get enabled. In this case I can see someone creating their configuration file with the current set of instrumentations. Then a couple of years later we offer more instrumentations out of the box, and the auto-instrumentation user wonders why they are not seeing that instrumentation being applied (especially if they are using the same or roughly the same configuration file template for all of their services). It's a departure from the behavior of our current environment variable, where if you specify nothing then you get all of the defaults. I'm not sure how to achieve that same behavior with this file based configuration approach.
Is there some way to remain in compliance with the current spec and offer some other experimental config that follows either the presets described in the configuration issue, or the approach used by Java in that issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment quite accurately describes why this approach shouldn’t be used.
In short, the file-based configuration was created to ensure that as little as possible is hidden somewhere in the code, and that when we make changes or updates, everything remains as it was for end users.
Personally, I also like the idea of presets, maybe it will be possible to push it into the specification.
And as soon as this feature appears, I’ll definitely add it.
zacharycmontoya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides some small non-blocking comments I wrote, one main question I have is whether we should make users list all integrations they want to use, or make all integrations on by default but configurable with an enabled flag. I'm leaning towards the second one, but I'd like to get your thoughts
Why
Towards #4394
Extracted and refactored from #4270
What
File based configuration for Instrumentations
Tests
Ci
Checklist
CHANGELOG.mdis updated.