-
Notifications
You must be signed in to change notification settings - Fork 566
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
4.x Several fixes to tracing config #8155
4.x Several fixes to tracing config #8155
Conversation
…d observe-style paths for built-in services to disabled-by-default tracing paths; change factory method and builder config method to accept common Config as parameter
+ " .build()))") | ||
List<PathTracingConfig> pathConfigs(); | ||
List<PathTracingConfig> paths(); |
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 is a backward incompatible change in public API.
We can either consider it a bugfix (and alignment to docs), and change it, or we could have both methods (deprecate the pathConfigs
and add a builder observer to correctly choose the right configured option)
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, I considered it a bug fix but I guess in my mind the real bug is the config key being path-configs
instead of paths
.
Another option: we could keep the old method name pathConfigs
but set the config key explicitly to paths
in the annotation.
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.
I think we need to keep the API backward compatible, so please either add a new method and deprecate existing (preferred option), or change the configuration key for this method.
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.
The most recent push sets the config key explicitly to "paths" and changes the method names back to what they were to avoid the backward-incompatible change to the APIs.
…n pathConfigs but set the config key to paths explicitly
Description
Resolves #8148
Fixes included:
path-configs
topaths
for config key within tracing to conform to the doc and to be more user-friendly.PathTracingConfig
to accept commonConfig
as parameter instead of the implConfig
type. This change is backward compatible with recompiling.Documentation
Bug fix. No doc change.