Enable extraction of http/unix adapter config from transport_options proc #1932
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ivoanjo
added a commit
that referenced
this pull request
Mar 7, 2022
…ise error In #1932 we did the groundwork required to support parsing the unix domain socket configuration from a c.tracing.transport_options configuration. Thus, we can now support unix domain sockets configured by the user without any other change (assuming that PR is merged before this commit is). The `transport_configuration_proc` can still contain weird transport configurations, but if that's the case, we just ignore them and print a warning. This is because we don't support every setting a user may provide in that proc. Worst case, for customers with a really custom config, is that the profiler won't be able to report data, and the customer will need to reach out to support for help updating it. (But we won't raise an exception or stop the profiler from starting).
luhenry
approved these changes
Mar 15, 2022
delner
added
core
delner
approved these changes
Mar 18, 2022
There was a problem hiding this comment.
We reviewed these PRs together, and the consensus we reached was that we should try to remove the proc for transport_options if we can implement a suitable replacement within the configuration settings code. This is the long term goal.
In the short term, to improve configuration usage, this is OK to do. Ideally this complexity will be removed long term when we have better transport configuration settings code in place.
Base automatically changed from
ivoanjo/agent-settings-resolver-cleanups
to
master
March 21, 2022 09:35
…proc As documented in the getting started guide (<https://docs.datadoghq.com/tracing/setup_overview/setup/ruby/#configuring-the-transport-layer>), one way of configuring ddtrace is to provide a proc as the `tracing.transport_options` configuration setting. Up until now, the `AgentSettingsResolver` class effectively "gave up" when it saw one of these procs, and just included it the proc in its `AgentSettings` output. This meant that: a) If there were any invalid or conflicting settings, we could not detect or warn about them b) We were in the dark on what settings actually were going to be used, since the proc could do anything it wanted. The `AgentSettings` object could be completely used or completely ignored. c) It was hard to provide alternative transport implementations, because they may not be able to implement the full adapter configuration API expected by the user code. For instance, this is needed to implement unix domain socket support in the new profiler libddprof-based transport. To fix this, I've implemented a very minimalistic approach: Inside the `AgentSettingsResolver` we try to execute the `transport_options` proc and see if all it does is configure an http or unix adapter. If so, success, we get its configuration and no longer need to deal with the proc. If the proc tries to do anything fancier (extra transport configuration or using a test or custom adapter), we just give up and retain the previous behavior. Worst case, the previous behavior is retained exactly. Best case, we are able to parse the configuration correctly, and are able to do the usual validations and include it in the `AgentSettings` object. Finally, yes, this does add more complexity to the `AgentSettingsResolver` but I claim that this complexity was already there, and now I've exposed it where it should be.
ivoanjo
force-pushed
the
ivoanjo/extend-agent-settings-parsing-to-transport-options
branch
from
2e0e8aa
to
a46c320
Compare
|
Pushed a rebase to solve a trivial conflict with #1930. |
|
Thanks for the review, merging away. CI is broken for |
ivoanjo
deleted the
ivoanjo/extend-agent-settings-parsing-to-transport-options
branch
ivoanjo
added a commit
that referenced
this pull request
May 16, 2022
…ise error In #1932 we did the groundwork required to support parsing the unix domain socket configuration from a c.tracing.transport_options configuration. Thus, we can now support unix domain sockets configured by the user without any other change (assuming that PR is merged before this commit is). The `transport_configuration_proc` can still contain weird transport configurations, but if that's the case, we just ignore them and print a warning. This is because we don't support every setting a user may provide in that proc. Worst case, for customers with a really custom config, is that the profiler won't be able to report data, and the customer will need to reach out to support for help updating it. (But we won't raise an exception or stop the profiler from starting).
ivoanjo
added a commit
that referenced
this pull request
May 17, 2022
…ise error In #1932 we did the groundwork required to support parsing the unix domain socket configuration from a c.tracing.transport_options configuration. Thus, we can now support unix domain sockets configured by the user without any other change (assuming that PR is merged before this commit is). The `transport_configuration_proc` can still contain weird transport configurations, but if that's the case, we just ignore them and print a warning. This is because we don't support every setting a user may provide in that proc. Worst case, for customers with a really custom config, is that the profiler won't be able to report data, and the customer will need to reach out to support for help updating it. (But we won't raise an exception or stop the profiler from starting).
ivoanjo
added a commit
that referenced
this pull request
May 19, 2022
…ise error In #1932 we did the groundwork required to support parsing the unix domain socket configuration from a c.tracing.transport_options configuration. Thus, we can now support unix domain sockets configured by the user without any other change (assuming that PR is merged before this commit is). The `transport_configuration_proc` can still contain weird transport configurations, but if that's the case, we just ignore them and print a warning. This is because we don't support every setting a user may provide in that proc. Worst case, for customers with a really custom config, is that the profiler won't be able to report data, and the customer will need to reach out to support for help updating it. (But we won't raise an exception or stop the profiler from starting).
ivoanjo
added a commit
that referenced
this pull request
May 25, 2022
…ise error In #1932 we did the groundwork required to support parsing the unix domain socket configuration from a c.tracing.transport_options configuration. Thus, we can now support unix domain sockets configured by the user without any other change (assuming that PR is merged before this commit is). The `transport_configuration_proc` can still contain weird transport configurations, but if that's the case, we just ignore them and print a warning. This is because we don't support every setting a user may provide in that proc. Worst case, for customers with a really custom config, is that the profiler won't be able to report data, and the customer will need to reach out to support for help updating it. (But we won't raise an exception or stop the profiler from starting).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As documented in the getting started guide (https://docs.datadoghq.com/tracing/setup_overview/setup/ruby/#configuring-the-transport-layer), one way of configuring ddtrace is to provide a proc as the
tracing.transport_optionsconfiguration setting.Up until now, the
AgentSettingsResolverclass effectively "gave up" when it saw one of these procs, and just included it the proc in itsAgentSettingsoutput.This meant that:
a) If there were any invalid or conflicting settings, we could not detect or warn about them
b) We were in the dark on what settings actually were going to be used, since the proc could do anything it wanted. The
AgentSettingsobject could be completely used or completely ignored.c) It was hard to provide alternative transport implementations, because they may not be able to implement the full adapter configuration API expected by the user code. For instance, this is needed to implement unix domain socket support in the new profiler libddprof-based transport.
To fix this, I've implemented a very minimalistic approach: Inside the
AgentSettingsResolverwe try to execute thetransport_optionsproc and see if all it does is configure an http or unix adapter. If so, success, we get its configuration andno longer need to deal with the proc.
If the proc tries to do anything fancier (extra transport configuration or using a test or custom adapter), we just give up and retain the previous behavior.
Worst case, the previous behavior is retained exactly. Best case, we are able to parse the configuration correctly, and are able to do the usual validations and include it in the
AgentSettingsobject.Finally, yes, this does add more complexity to the
AgentSettingsResolverbut I claim that this complexity was already there, and now I've exposed it where it should be.(P.s.: This relies on the refactoring in #1931)