-
Notifications
You must be signed in to change notification settings - Fork 579
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 metrics app name tag fix #9665
4.x metrics app name tag fix #9665
Conversation
…ests so they properly initialize the system tags manager which governs the addition of tags
default Map<String, String> displayTagPairs() { | ||
Map<String, String> result = new TreeMap<>(); | ||
displayTags().forEach(tag -> result.put(tag.key(), tag.value())); | ||
return result; |
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.
Should this be unmodifiable?
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.
What would this being unmodifiable protect against? The returned Map
does not use the original for backing.
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.
True enough. Although without the unmodifiable contract, then it is OK for any caller to do, for example, something stupid like stm.displayTagPairs().clear()
. Obviously that's their problem, not yours, but it's part of the contract. (The alternative is documenting that it will throw an UnsupportedOperationException
if a mutation is applied to the returned Map
.)
|
||
@Override | ||
public Map<String, String> displayTagPairs() { | ||
return Map.copyOf(systemTagPairs); |
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.
A copy? Or would an unmodifiable view suffice?
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.
No need for a copy here. Changed.
Similarly changed displayTags()
which also does not need a copy but can instead returns an unmodifiable view.
Description
Resolves #9664
The fix for the main bug exposed another problem (see below) which this PR also fixes.
The main fix
Synopsis: Helidon failed to correctly provide in SE the tag to be used for conveying the app name if the user specified an app name in the config (but not explicitly the tag name to use).
Details:
Metrics has a
MetricsProgrammaticConfig
interface which is primarily a way for SE and MP to dictate config settings in theMetricsConfig
type. (Even if the user sets them, the implementations of this interface override those settings.) Among other things, this type allows control over reserved tag names, the tag name used to convey the meter's scope, etc.The pre-existing MP implementation is fine. There was no SE implementation, relying on (incomplete) default behavior defined in the interface itself.
This PR introduces, for clarity as well as the bug fix, an explicit SE-specific implementation. It is always consulted, but if the MP one is also present the code runs it last, overriding anything the SE implementation might have assigned.
These implementations are invoked when the
MetricsFactoryManager
is invoked with aMetricsConfig
instance to prepare aMetricsFactory
(which, ultimately, instantiates aMeterRegistry
which is where some of the settings go into effect.) There is a slight change in that class where it collects theMetricsProgrammaticConfig
instances to apply them so that the MP implementation supersedes the SE one.The other problem: recursive infinite loop exposed
Synopsis:
SystemTagsManagerImpl
is initialized byMetricsFactory
, and during its initialization it would invokeMetricsFactory
which would attempt to re-initializeSystemTagsManagerImpl
, and around we go.In metrics, the
MetricsFactory
is a gateway to many operations whose implementations depend on the underlying metrics provider (Micrometer is the only one so far). In obtaining an instance of this type, the system also prepares an instance ofSystemTagsManager
. One responsibility of that type is to provide a collection of HelidonTag
s containing whatever tags should be applied to all meters created.Before this PR, that code would prepare the
Tag
collection during initialization. The latent bug (hidden by the main bug) was that this initialization code would invokeTag.create
(on the Helidon-neutralTag
interface) which has to ensure that the tag names from the caller (usually the app developer) do not conflict with the reserved tags, which in turn would need to access theMetricsFactory
which was currently being initialized so theLazyValue
holding it was still empty...triggering a recursive construction.The fix to this bug is to store a
Map<String, String>
for the universal tag names and values during initialization and only manufactureTag
instances upon first real need...after initialization has completed. This new map removes the need for the separatesystemTagNames
set (because it can be derived from the key set of the newMap<String, String>
.There was one other bit of code that referred to the
instance()
method instead of theinstance
parameter which caused a separate recursive infinite loop, also fixed.Documentation
No doc impact.